Code review comment for lp:~yao-codesourcery/gcc-linaro/4.4-fix-647597

Revision history for this message
Yao Qi (yao-codesourcery) wrote :

> I'm wondering why you need the "if" here:
>
> 24 + if (DECL_SOURCE_LOCATION (decl) == UNKNOWN_LOCATION)
> 25 + DECL_SOURCE_LOCATION (decl) = loc;
>
> Given that immediately before the call to (*make_fname_decl), input_location
> is unconditionally set to UNKNOWN_LOCATION, DECL_SOURCE_LOCATION (decl) should
> *always* be UNKNOWN_LOCATION here ...
>
> Do you have any case where this DECL_SOURCE_LOCATION is something different
> here? If not, it seems to me it would be less confusing to leave the "if" out
> and unconditionally set the location.
>

No, I don't have such case. The reason I put "if" here is that I am little worried
that input_location is changed somewhere else, after it is set to UNKNOWN_LOCATION.
Because global variable input_location is modified in many places in current c/cpp
front end.

I've removed that "if" statement. Regression tested again on x86_64. Rebuild ARM cross compiler, no segmentation fault. I'll check in.

> Note that in current mainline, make_fname_decl is changed to take a location_t
> argument, which is then used to set the decl's location. This was done as
> part of the big diagnostics branch merge:
> http://gcc.gnu.org/ml/gcc-cvs/2009-06/msg00422.html
> So it seems your patch is doing the right thing.

I planed to backport this big patch before, but gave up after some hours, since it is not
as easy as I firstly imagined.

« Back to merge proposal