sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Great!
The overloading impl is a little surprising to me.
I was assuming that object would always win over function (or that it would be
disallowed to combine the two).
I think that is simpler to implement, as you know in advance what type of
expansion you're attempting.
But what you have isn't that much more complicated, and the extra flexibility
may prove useful.
One weird side-effect: if you define an object-like macro FOO and then write
`FOO(` and never close the paren, then we die inside macro-arg-parsing rather
than hitting the normal EOF-while-formatting behavior. I left a comment on
parseMacroCall() about how to fix this but I'm not sure whether it matters at a
high level.
================
Comment at: clang/lib/Format/MacroExpander.cpp:172
assert(defined(ID->TokenText));
+ assert(
+ (!OptionalArgs && ObjectLike.find(ID->TokenText) != ObjectLike.end()) ||
----------------
this assertion failure isn't going to be as useful as it could be!
maybe rather
```
if (OptionalArgs)
assert(...);
else
assert(...);
```
Also I think we're now ensuring to only call this if arity matches, so I'd make
this a precondition and replace the first assert with hasArity to make it
easier to reason about
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4580
+ LLVM_DEBUG({
+ llvm::dbgs() << "Call: " << ID->TokenText << "(";
+ if (Args) {
----------------
nit: call => Macro call in dbgs()?
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4592
+ !Macros.hasArity(ID->TokenText, Args->size())) {
+ // The macro is overloaded to be both object-like and function-like,
+ // but none of the function-like arities match the number of arguments.
----------------
Strictly I don't think this comment is true, we hit this path when it's
**only** an object-like macro, used before parens.
For this reason you might *not* want the dbgs() here but rather in the bottom
`else`
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4610
+ Unexpanded[ID] = std::move(UnexpandedLine);
+ SmallVector<FormatToken *, 8> New = Macros.expand(ID, Args);
+ if (!New.empty())
----------------
(nit: std::move(args)?)
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4610
+ Unexpanded[ID] = std::move(UnexpandedLine);
+ SmallVector<FormatToken *, 8> New = Macros.expand(ID, Args);
+ if (!New.empty())
----------------
sammccall wrote:
> (nit: std::move(args)?)
nit: maybe "expansion" is more descriptive than "new"?
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4620
+ });
+ } else {
+ Tokens->setPosition(Position);
----------------
I think you may want a comment and/or dbgs() here explaining this case: `Didn't
expand macro FOO because it was used {with 3|without} args, which doesn't match
any definition`
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4696
+ } while (!eof());
+ return {};
+}
----------------
I believe this has changed meaning along with the return type: previously it
returned an empty vector, now it returns `nullopt`. Not sure which you intend
here.
I think the ideal thing here would be to **rewind the token stream back to
before the `(`**, and return `nullopt`. We'll end up performing an object-like
substitution if possible, and non-macro parsing will continue.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144170/new/
https://reviews.llvm.org/D144170
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
