sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Ship it!
================
Comment at: clang/lib/Format/FormatToken.h:177
+/// EndOfExpansion: 0 0 0 2 1 0 1
+struct MacroContext {
+ MacroContext(MacroRole Role) : Role(Role) {}
----------------
klimek wrote:
> sammccall wrote:
> > "context" is often pretty vague - "MacroSource" isn't a brilliant name but
> > at least seems to hint at the direction (that the FormatToken is the
> > expanded token and the MacroSource gives information about what it was
> > expanded from)
> >
> > I don't feel strongly about this though, up to you.
> MacroSource sounds like it is about the macro source (i.e. the tokens written
> for the macro).
> I'd be happy to rename to MacroExpansion or MacroExpansionInfo or somesuch if
> you think that helps?
Oops, accidental "source" pun. MacroOrigin would be another name in the same
spirit. But MacroExpansion sounds good to me, too.
================
Comment at: clang/lib/Format/MacroExpander.cpp:190
+ return true;
+ for (const auto &Arg : Args[I->getValue()]) {
+ // A token can be part of a macro argument at multiple levels.
----------------
nit: this is confusingly a const reference to a non-const pointer... `auto *`
or `FormatToken *`?
================
Comment at: clang/lib/Format/MacroExpander.cpp:142
+ auto Definition = Parser.parse();
+ Definitions[Definition.Name] = Definition;
+ }
----------------
klimek wrote:
> sammccall wrote:
> > nit: this is a copy for what seems like no reason - move `Parser.parse()`
> > inline to this line?
> Reason is that we need the name.
oops, right.
std::move() the RHS? (mostly I just find the copies surprising, so draws
attention)
================
Comment at: clang/unittests/Format/MacroExpanderTest.cpp:183
+ EXPECT_ATTRIBUTES(Result, Attributes);
+}
+
----------------
may want a test that uses of an arg after the first are not expanded, because
that "guards" a bunch of nasty potential bugs
================
Comment at: clang/unittests/Format/TestLexer.h:15
+
+#ifndef CLANG_UNITTESTS_FORMAT_TEST_LEXER_H
+#define CLANG_UNITTESTS_FORMAT_TEST_LEXER_H
----------------
I guess clang-tidy wants ..._TESTLEXER_H here
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83296/new/
https://reviews.llvm.org/D83296
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
