[PATCH] D83296: [clang-format] Add a MacroExpander.

[Sam McCall via Phabricator via cfe-commits]

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