Last Comment Bug 764286 - Remove unused AC_SUBSTs
: Remove unused AC_SUBSTs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mike Hommey [:glandium]
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks: 742795
  Show dependency treegraph
 
Reported: 2012-06-13 00:33 PDT by Mike Hommey [:glandium]
Modified: 2012-06-20 08:02 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove unused AC_SUBSTs (15.08 KB, patch)
2012-06-13 03:06 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Remove unused AC_SUBSTs (15.22 KB, patch)
2012-06-13 04:04 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2012-06-13 00:33:30 PDT
Bug 763987 is going to make autoconf.mk autogenerated. As such, any AC_SUBST that is defined and that is not actually used in autoconf.mk and any other file listed in AC_OUTPUT may hurt. As a matter of fact, at least STL_FLAGS in js/src does.
Comment 1 Mike Hommey [:glandium] 2012-06-13 00:34:51 PDT
When I was saying bug 763987, I really meant bug 742795.
Comment 2 Mike Hommey [:glandium] 2012-06-13 03:06:18 PDT
Created attachment 632619 [details] [diff] [review]
Remove unused AC_SUBSTs

That just removes the AC_SUBSTs that have no use in autoconf.mk and other config.status-generated stuff. I'm pretty sure there are plenty variables defined in autoconf.mk but never used, but I didn't go after those.
Comment 3 Mike Hommey [:glandium] 2012-06-13 04:04:54 PDT
Created attachment 632631 [details] [diff] [review]
Remove unused AC_SUBSTs

Without a syntax problem after iconv check removal.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2012-06-18 12:38:18 PDT
Comment on attachment 632631 [details] [diff] [review]
Remove unused AC_SUBSTs

Review of attachment 632631 [details] [diff] [review]:
-----------------------------------------------------------------

r- because I don't understand a few of these large code removals.

::: configure.in
@@ -809,5 @@
>  AC_SUBST(GNU_CXX)
>  AC_SUBST(INTEL_CC)
>  AC_SUBST(INTEL_CXX)
> -AC_SUBST(CLANG_CC)
> -AC_SUBST(CLANG_CXX)

Huh, we don't actually use these? I guess we can always add them back if we need them.

::: js/src/configure.in
@@ -574,5 @@
>          fi
>  
>          unset _MSVC_VER_FILTER
> -
> -        AC_CACHE_CHECK(for std::_Throw, ac_cv_have_std__Throw,

Why are you removing this hunk? That doesn't seem right.

@@ -3043,5 @@
> -        [ac_cv_func_iconv=no]
> -        )]
> -    )
> -if test "$ac_cv_func_iconv" = "yes"; then
> -    AC_DEFINE(HAVE_ICONV)

It looks like this define actually does get used, why is this getting removed?
Comment 5 Mike Hommey [:glandium] 2012-06-18 22:54:56 PDT
(In reply to Ted Mielczarek [:ted] from comment #4)
> Comment on attachment 632631 [details] [diff] [review]
> Remove unused AC_SUBSTs
> 
> Review of attachment 632631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because I don't understand a few of these large code removals.
> 
> ::: configure.in
> @@ -809,5 @@
> >  AC_SUBST(GNU_CXX)
> >  AC_SUBST(INTEL_CC)
> >  AC_SUBST(INTEL_CXX)
> > -AC_SUBST(CLANG_CC)
> > -AC_SUBST(CLANG_CXX)
> 
> Huh, we don't actually use these? I guess we can always add them back if we
> need them.

We only use them to trigger other tests during configure.

> ::: js/src/configure.in
> @@ -574,5 @@
> >          fi
> >  
> >          unset _MSVC_VER_FILTER
> > -
> > -        AC_CACHE_CHECK(for std::_Throw, ac_cv_have_std__Throw,
> 
> Why are you removing this hunk? That doesn't seem right.

The AC_SUBST corresponding to the test is not used in js/src. So it's not useful to keep the test.
 
> @@ -3043,5 @@
> > -        [ac_cv_func_iconv=no]
> > -        )]
> > -    )
> > -if test "$ac_cv_func_iconv" = "yes"; then
> > -    AC_DEFINE(HAVE_ICONV)
> 
> It looks like this define actually does get used

Not in js/src.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2012-06-19 05:19:33 PDT
Oh, js/src/, duh.
Comment 7 Ted Mielczarek [:ted.mielczarek] 2012-06-19 05:36:07 PDT
Comment on attachment 632631 [details] [diff] [review]
Remove unused AC_SUBSTs

Review of attachment 632631 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray for reading comprehension.

Note You need to log in before you can comment on or make changes to this bug.