This is an archive of the discontinued LLVM Phabricator instance.

clang-format: fix block OpeningLineIndex around preprocessor
ClosedPublic

Authored by Typz on Jul 17 2017, 6:36 AM.

Details

Summary

The current code would return an incorrect value when a preprocessor
directive is present immediately after the opening brace: this causes
the nanespace end comment fixer to break in some places, for exemple it
would not add the comment in this case:

namespace a {
#define FOO
}

Fixing the computation is simple enough, but it was breaking a feature,
as it would cause comments to be added also when the namespace
declaration was dependant on conditional compilation.

To fix this, a hash of the current preprocessor stack/branches is
computed at the beginning of parseBlock(), so that we explicitely do not
store the OpeningLineIndex when the beginning and end of the block are
not in the same preprocessor conditions.

Tthe hash is computed based on the line, but this could propbably be
improved by using the actual condition, so that clang-format would be
able to match multiple identical #ifdef blocks.

Event Timeline

Typz created this revision.Jul 17 2017, 6:36 AM
krasimir edited edge metadata.Jul 17 2017, 1:06 PM

Nice!

lib/Format/UnwrappedLineParser.cpp
461

@djasper: do you aware of some baked-in hash-combine functionality in llvm which this can use directly? If there is no, I'm happy with this.

unittests/Format/NamespaceEndCommentsFixerTest.cpp
556

Maybe also add some negative tests?

Typz updated this revision to Diff 107030.Jul 18 2017, 1:40 AM
Typz marked an inline comment as done.

Add more tests

krasimir added inline comments.Jul 20 2017, 2:47 AM
lib/Format/UnwrappedLineParser.cpp
464

When I patch this, I get an UnwrappedLineParser.cpp:457:16 error: implicit instantiation of undefined template 'std::hash<clang::format::UnwrappedLineParser::PPBranchKind>'.

lib/Format/UnwrappedLineParser.h
158

Please add a short comment of why is the preprocessor hash needed.

213

This operator== is confusing. Please remove it.

Typz updated this revision to Diff 107843.Jul 23 2017, 2:33 PM
Typz marked 3 inline comments as done.

Address review commentsx

Typz added inline comments.Jul 24 2017, 5:42 AM
lib/Format/UnwrappedLineParser.cpp
464

Seems to work fine with latest Ubuntu, but not on macos.

krasimir accepted this revision.Jul 24 2017, 1:06 PM

Looks good! Thanks!
Please rebase with master before committing since we've been touching the same lines in UnwrappedLineParser.cpp.

This revision is now accepted and ready to land.Jul 24 2017, 1:06 PM
This revision was automatically updated to reflect the committed changes.