This is an archive of the discontinued LLVM Phabricator instance.

Comment parsing: Allow inline commands to have 0 or more than 1 argument
ClosedPublic

Authored by aaronpuchert on May 11 2022, 4:22 PM.

Details

Summary

That's required to support \n, but can also used for other commands.
The argument parsing is basically duplicated from block commands, but we
can't easily factor that out as they have a different argument data
type.

This should fix #55319.

Diff Detail

Event Timeline

aaronpuchert created this revision.May 11 2022, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 4:22 PM
aaronpuchert requested review of this revision.May 11 2022, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 4:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaronpuchert added inline comments.May 11 2022, 4:32 PM
clang/include/clang/AST/Comment.h
303

Removing that allows me to build an array without initializing all members right away. Alternative would be to add a default constructor here.

clang/include/clang/Basic/DiagnosticCommentKinds.td
159

If you find that %plural too fancy, a plain %2 should also do.

clang/lib/AST/CommentParser.cpp
291–304

This is basically what I'm duplicating. As it happens, the two Argument structures are structurally the same, so we could unify them and factor out this code. I'd probably do that in a separate change though (prior to this or as follow-up).

clang/test/Headers/x86-intrinsics-headers-clean.cpp
4

@RKSimon, according to https://github.com/llvm/llvm-project/issues/35297 you mostly wanted -pedantic, but I took the liberty of enabling both. They're currently coupled (similar to https://github.com/llvm/llvm-project/issues/19442).

gribozavr2 accepted this revision.May 12 2022, 4:22 AM
gribozavr2 added inline comments.
clang/include/clang/AST/CommentCommands.td
94

Could you add a test that shows that the text after \n is not treated as the argument?

You could verify the comment AST like here: llvm-project/clang/test/AST/ast-dump-comment.cpp

This revision is now accepted and ready to land.May 12 2022, 4:22 AM
aaronpuchert marked an inline comment as done.May 12 2022, 9:19 AM
aaronpuchert added inline comments.
clang/include/clang/AST/CommentCommands.td
94

That's a good idea, I'll do that.

aaronpuchert marked an inline comment as done.

Unify Argument types and parsing. Add AST test. Simplify TableGen.

gribozavr2 accepted this revision.May 12 2022, 10:10 AM
thakis added a subscriber: thakis.

Hello, this broke building on mac:

Please take a look, and revert for now if it takes a while to fix.

(Here's the error:

In file included from ../../clang/lib/AST/CommentParser.cpp:9:
In file included from ../../clang/include/clang/AST/CommentParser.h:16:
In file included from ../../clang/include/clang/AST/Comment.h:17:
In file included from ../../clang/include/clang/AST/DeclObjC.h:16:
In file included from ../../clang/include/clang/AST/Decl.h:19:
In file included from ../../clang/include/clang/AST/DeclBase.h:18:
In file included from ../../clang/include/clang/AST/DeclarationName.h:16:
In file included from ../../clang/include/clang/AST/Type.h:21:
In file included from ../../clang/include/clang/AST/NestedNameSpecifier.h:18:
../../clang/include/clang/Basic/Diagnostic.h:1353:8: error: use of overloaded operator '<<' is ambiguous (with operand types 'const clang::StreamingDiagnostic' and 'typename remove_reference<unsigned long &>::type' (aka 'unsigned long'))
    DB << std::move(V);
    ~~ ^  ~~~~~~~~~~~~
../../clang/lib/AST/CommentParser.cpp:417:57: note: in instantiation of function template specialization 'clang::DiagnosticBuilder::operator<<<unsigned long, void>' requested here
        << CommandTok.is(tok::at_command) << Info->Name << Args.size()
                                                        ^
../../clang/include/clang/Basic/Diagnostic.h:1400:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                  ^
../../clang/include/clang/Basic/Diagnostic.h:1406:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                  ^
../../clang/include/clang/Basic/Diagnostic.h:1422:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                  ^
../../clang/include/clang/Basic/Diagnostic.h:1428:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                  ^
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(int, unsigned long)
    DB << std::move(V);
       ^
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(int, int)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(int, long)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(int, long long)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(int, __int128)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(int, unsigned int)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(int, unsigned long long)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(int, unsigned __int128)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(long, unsigned long)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(long long, unsigned long)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(__int128, unsigned long)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(unsigned int, unsigned long)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(unsigned long, unsigned long)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(unsigned long long, unsigned long)
../../clang/include/clang/Basic/Diagnostic.h:1353:8: note: built-in candidate operator<<(unsigned __int128, unsigned long)

)

Hmm, didn't see this on Linux. I'll try to disambiguate.

So we have overloads

inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, int I);
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, int64_t I);
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, unsigned I);
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, uint64_t I);

And it appears that unsigned long does not have a clear favorite here. Is that because long is 32-bit like on Windows, but at the same time it's not the same as unsigned which is also 32-bit? Could we resolve this somehow so that we don't have the next person writing inconspicuous code to run into this?

Perhaps we should overload on (u)int32_t instead int/unsigned?

https://github.com/llvm/llvm-project/commit/25862f53cce966cef2957825095861dec631d4f1 fixed the build, thanks!

No opinion off the top of my head on changing the signature. Might be a good idea, maybe?

And it appears that unsigned long does not have a clear favorite here.

Found it on StackOverflow: https://stackoverflow.com/questions/11603818/why-is-there-ambiguity-between-uint32-t-and-uint64-t-when-using-size-t-on-mac-os.

Is that because long is 32-bit like on Windows, but at the same time it's not the same as unsigned which is also 32-bit?

No. Mac is LP64, but uses long long for (u)int64_t.

Perhaps we should overload on (u)int32_t instead int/unsigned?

Hence this doesn't solve it. I'm thinking that perhaps we should overload on int, long, long long. Seems silly, but C++ defines them as different types. Proposed this in D125580.

Proposed this in D125580.

Landed this two days ago and no one complained about it so far, so let's hope that solved it.