Re: [PATCH 2/9] ENABLE_CHECKING refactoring: libcpp

[Jeff Law <law@redhat.com>]

On 10/06/2015 06:40 AM, Bernd Schmidt wrote:
> I'm not entirely sure what to make of this series. There seem to be good
> bits in there but also some things I find questionable. I'll add some
> comments on things that occur to me.
Maybe we should start pulling out the bits that we think are ready & 
good and start installing them independently.

I'm less concerned about getting conditional compilation out of the lib* 
directories right now than I am the core of the compiler.  So I wouldn't 
lose any sleep if we extracted the obviously good bits for libcpp tested 
& installed those, then table the rest of the libcpp stuff and focused 
on the core compiler.

Thoughts?


>
> On 10/06/2015 01:28 AM, Mikhail Maltsev wrote:
>>
>>     * include/line-map.h: Fix use of ENABLE_CHECKING.
>
> Fix how? What's wrong with it?
>
>>   /* Sanity-checks are dependent on command-line options, so it is
>>      called as a subroutine of cpp_read_main_file ().  */
>> -#if ENABLE_CHECKING
>> +#if CHECKING_P
>>   static void sanity_checks (cpp_reader *);
>>   static void sanity_checks (cpp_reader *pfile)
>>   {
>
> Ok, this one seems to be a real problem (should have been #ifdef), but...
Agreed.

>
>> -#ifdef ENABLE_CHECKING
>> +#if CHECKING_P
>
> I fail to see the point of this change.
I'm guessing (and Mikhail, please correct me if I'm wrong), but I think 
he's trying to get away from ENABLE_CHECKING and instead use a macro 
which is always defined to a value.


>
>> -#ifdef ENABLE_CHECKING
>> -      if (kind == MACRO_ARG_TOKEN_STRINGIFIED
>> -      || !track_macro_exp_p)
>> -    /* We can't set the location of a stringified argument
>> -       token and we can't set any location if we aren't tracking
>> -       macro expansion locations.   */
>> -    abort ();
>> -#endif
>> +      /* We can't set the location of a stringified argument
>> +     token and we can't set any location if we aren't tracking
>> +     macro expansion locations.   */
>> +      gcc_checking_assert (kind != MACRO_ARG_TOKEN_STRINGIFIED
>> +               && track_macro_exp_p);
>
> This kind of change seems good. I think the patch series would benefit
> if it was separated thematically rather than by sets of files. I.e.,
> merge all changes like this one into one patch or maybe a few if it
> grows too large.
That would work for me.  I just asked Mikhail to try and break down the 
monster patch into something that could be digested.  Ultimately my goal 
was to make it possible to start reviewing and installing these bits and 
keep making progress on removing the conditionally compiled code.

>
>> +/* Redefine abort to report an internal error w/o coredump, and
>> +   reporting the location of the error in the source file.  */
>> +extern void fancy_abort (const char *, int, const char *)
>> ATTRIBUTE_NORETURN;
>> +#define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__)
>> +
>> +/* Use gcc_assert(EXPR) to test invariants.  */
>> +#if ENABLE_ASSERT_CHECKING
>> +#define gcc_assert(EXPR)                         \
>> +   ((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, __FUNCTION__),
>> 0 : 0))
>> +#elif (GCC_VERSION >= 4005)
>> +#define gcc_assert(EXPR)                         \
>> +  ((void)(__builtin_expect (!(EXPR), 0) ? __builtin_unreachable (), 0
>> : 0))
>> +#else
>> +/* Include EXPR, so that unused variable warnings do not occur.  */
>> +#define gcc_assert(EXPR) ((void)(0 && (EXPR)))
>> +#endif
>
> Probably a good thing, but it looks like libcpp has grown its own
> variant linemap_assert; we should check whether that can be replaced.
>
> Also, the previous patch already introduces a use of gcc_assert, or at
> least a reference to it, and it's only defined here. The two
> modifications of libcpp/system.h should probably be merged into one.
Agreed.

jeff