Hello Nicholas, Nicholas Ormrod <nicholas.orm...@hotmail.com> 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