This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] throws an incorrect assertion in consumeToken() formatting the MSVC stl
ClosedPublic

Authored by MyDeveloperDay on Oct 9 2019, 8:25 AM.

Details

Summary

An incorrect assertion is thrown when clang-formatting MSVC's STL library

Assertion failed: !Line.startsWith(tok::hash), file C:/llvm/llvm-project/clang/lib/Format/TokenAnnotator.cpp, line 847
Stack dump:
0.      Program arguments: C:\llvm\build\bin\clang-format.exe -i -n ./stl/inc/xkeycheck.h
#if defined(while)
#define while EMIT WARNING C4005
#error The C++ Standard Library forbids macroizing the keyword "while". \
Enable warning C4005 to find the forbidden define.
#endif // while

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Oct 9 2019, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 8:25 AM
MyDeveloperDay edited the summary of this revision. (Show Details)Oct 9 2019, 8:26 AM
clang/lib/Format/TokenAnnotator.cpp
854

It's not clear to me whether or not the token should be consumed. The previous assertion leads me to think no, and in that case, I think this should be

if (Line.startsWith(tok::hash))
    return false;

A comment on this would also be helpful.

MyDeveloperDay marked an inline comment as done.

I'm not even sure if the assertion is valid?

mitchell-stellar added a comment.EditedOct 9 2019, 9:58 AM

To me, whether or not the assertion was valid is irrelevant. That assertion was intentionally added, and its replacement with an if() statement materially changes the inputs and outputs of the function. I'm questioning whether the input of a "while" token within a preprocessor statement should output true or false. (Before, it didn't output anything; it errored.) Does this make sense? I'm just asking for clarification on this change.

MyDeveloperDay added inline comments.Oct 9 2019, 10:28 AM
clang/lib/Format/TokenAnnotator.cpp
854

to be honest I'm not sure what the assertion is trying to actually assert (I'm not a massive fan of assertions especially like this), just because I see while am I not expecting to see a # at the beginning of the line?

I've tried both yours, mine and removing it completely and none of the test fail, so I feel like imposing the "Beyonce rule!!" and getting rid of it.

mitchell-stellar accepted this revision.Oct 9 2019, 11:07 AM

Okay.

LGTM.

This revision is now accepted and ready to land.Oct 9 2019, 11:07 AM
This revision was automatically updated to reflect the committed changes.