Hello Nicholas,
Nicholas Ormrod <[email protected]> writes:
> I found time this morning to run your changes through our system. I
> patched our gcc-4.8.1 with your latest change, and ran it through our
> folly testsuite.
Thanks!
> One thing that I immediately noticed was that this increased the preprocessed
> size substantially.
> When preprocessing my favorite .cpp file, its .ii grew from 137k lines
> to 145k, a 5% increase.
Yeah, the growth is expected. It's interesting to have actual numbers,
thank you for that.
> All the folly code compiled and ran successfully under the changes.
>
> I looked at some of the preprocessed output. I was pleased to see that
> consecutive macros that expanded entirely to system tokens did not
> insert unnecessary line directives between them.
Cool!
> I did, however, notice that __LINE__ was treated as belonging to the
> calling file, even when its token appears in the system file. That is
> to say:
>
> CODE:
>
> // system macro
> #define FOO() sys_token __LINE__ sys_token
>
> // non-system callsite
> FOO()
>
> // preprocessed output
> # 3 "test.cpp" 3 4
> sys_token
> # 3 "test.cpp"
> 3
> # 3 "test.cpp" 3 4
> sys_token
>
> :CODE
Yeah. For Built-in tokens that are expanded like that we only do track
their the location of their expansion, not their spelling location. So
this behaviour is expected as well.
> This seems to generalize to other builtin macros, like __FILE__.
Indeed.
>
> Otherwise, the code looks fine. There is only one thing I noticed:
>
>> + if (do_line_adjustments
>> + && !in_pragma
>> + && !line_marker_emitted
>> + && print.prev_was_system_token != !!in_system_header_at(loc))
>> + /* The system-ness of this token is different from the one
>> + of the previous token. Let's emit a line change to
>> + mark the new system-ness before we emit the token. */
>> + line_marker_emitted = do_line_change (pfile, token, loc, false);
>
> This line_marker_emitted assignment is immediately overwritten, two lines
> below. However, from a
> maintainability perspective, this is probably a good assignment to
> keep.
Yeah, maintainability is why I kept it. But if the maintainers feel
strongly about it I can, certainly just remove that assignment.
Thank you for your time on this!
Cheers.
--
Dodji
