[PATCH] D144170: [clang-format] Add simple macro replacements in formatting.

[Sam McCall via Phabricator via cfe-commits]

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