sammccall added a comment.
Still LG
================
Comment at: clang/lib/Format/MacroExpander.cpp:172
assert(defined(ID->TokenText));
+ assert(
+ (!OptionalArgs && ObjectLike.find(ID->TokenText) != ObjectLike.end()) ||
----------------
klimek wrote:
> sammccall wrote:
> > 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
> Hopefully done, the comment took me a while to parse :)
Sorry about that, LG though
================
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.
----------------
klimek wrote:
> sammccall wrote:
> > 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`
> I'm pretty sure we hit this when it's overloaded for both, but we call it
> with the wrong arity.
> E.g. A=x, A(x, y, z)=x y z
>
> A(1, 2)
> - Macros.objectLike(A) -> true
> - Args -> true
> - !Macros.hasArity(A, 2) -> true
I agree that (the case described in the comment) => (we get to this point in
the code).
I'm disputing that (we get to this point in the code) => (the case described in
the comment).
i.e. given `A=x`, code=`A(1, 2)`, we also get: `objectLike(A) -> true`, `Args
-> true`, `!Macros.hasArity(A, 2) -> true`, but this time the comment is lying
about the state.
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
