[PATCH] D54405: Record whether a AST Matcher constructs a Node

[Mailing List "cfe-commits" via Phabricator via cfe-commits]

cfe-commits added a subscriber: aaron.ballman.
cfe-commits added a comment.

I don’t really have much more to add here except to refer you to the style
guide:

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Specifically this line: “Use auto if and only if it makes the code more
readable or easier to maintain.”

Given that 2 out of 2 reviewers who have looked at this have said it did
not make the code more readable or easier to maintain for them , and have
further said that they feel the return type is not “obvious from the
context”, i do not believe this usage fits within our style guidelines.

I don’t think it’s worth beating on this anymore though, because this is a
lot of back and forth over something that doesn’t actually merit this much
discussion. So in the interest of conforming to the style guide above,
please remove auto and then start a thread on llvm-dev if you disagree with
the policy

- F7542440: msg-532-122.txt 


Repository:
  rC Clang

https://reviews.llvm.org/D54405



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

[Mailing List "cfe-commits" via Phabricator via cfe-commits]

cfe-commits added a comment.



- F7238787: msg-7222-125.txt 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51847: Print correctly dependency paths on Windows

[Mailing List "cfe-commits" via Phabricator via cfe-commits]

cfe-commits added a comment.

Lgtm

- F7184192: msg-6991-166.txt 


https://reviews.llvm.org/D51847



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Mailing List &quot;cfe-commits&quot; via Phabricator via cfe-commits]

cfe-commits added a subscriber: junbuml.
cfe-commits added a comment.

I think you and Richard agreed that you weren’t going to synthesize a whole
expression tree at every use of the operator, and I agree with that
decision. That’s very different from what I’m asking you to do, which is to
synthesize in isolation a call to the copy-constructor. There are several
places in the compiler that require these implicit copies which aren’t just
normal expressions; this is the common pattern for handling them. The
synthesized expression can be emitted multiple times, and it can be freely
re-synthesized in different translation units instead of being serialized.
You’re already finding and caching a constructor; storing a
CXXConstructExpr is basically thr same thing, but in a nicer form that
handles more cases and doesn’t require as much redundant code in IRGen.

STLs *frequently* make use of default arguments on copy constructors (for
allocators). I agree that it’s unlikely that that would happen here, but
that’s precisely because it’s unlikely that this type would ever be
non-trivial.

Mostly, though, I don’t understand the point of imposing a partial set of
non-conformant restrictions on the type. It’s easy to support an arbitrary
copy constructor by synthesizing a CXXConstructExpr, and this will
magically take care of any constexpr issues, as well as removing the need
for open-coding a constructor call.

The constexpr issues are that certain references to constexpr variables of
literal type (as these types are likely to be) are required to not ODR-use
the variable but instead just directly produce the initializer as the
expression result.  That’s especially important here because (1) existing
STL binaries will not define these variables, and we shouldn’t create
artificial deployment problems for this feature, and (2) we’d really rather
not emit these expressions as loads from externally-defined variables that
the optimizer won’t be able to optimize.

John.

Eric Fiselier via Phabricator  wrote:

> EricWF added inline comments.
> 
> 
>  Comment at: lib/Sema/SemaDeclCXX.cpp:8931
>  +  /*ConstArg*/ true, false, false, false, false);
>  +  auto *CopyCtor = cast_or_null(SMI.getMethod());
>  +
> 
>  
> 
> rjmccall wrote:
>  > Sorry, I didn't realize you'd go off and write this code manually.
>  >
>  > The way we generally handle this sort of thing is just to synthesize an
>  expression in Sema that performs the copy-construction.  That way, the
>  stdlib can be as crazy as it wants — default arguments on the
>  copy-constructor or whatever else — and it just works.  The pattern to
>  follow here is the code in Sema::BuildExceptionDeclaration, except that in
>  your case you can completely dispense with faking up an OpaqueValueExpr and
>  instead just build a DeclRefExpr to the actual variable.  (You could even
>  use ActOnIdExpression for this, although just synthesizing the DRE
>  shouldn't be too hard.)  Then the ComparisonCategoryInfo can just store
>  expressions for each of the three (four?) variables, and in IRGen you just
>  evaluate the appropriate one into the destination.
>  I think this goes against the direction Richard and I decided to go, which
>  was to not synthesize any expressions.
> 
> The problem is that the synthesized expressions cannot be stored in
>  `ComparisonCategoryInfo`, because the data it contains is not serialized.
>  So when we read the AST back, the `ComparisonCategoryInfo` will be empty.
>  And for obvious reasons we can't cache the data in Sema either.
>  Additionally, we probably don't want to waste space building and storing
>  synthesized expressions in each AST node which needs them.
> 
> Although the checking here leaves something to be desired, I suspect it's
>  more than enough to handle any conforming STL implementation.
> 
> https://reviews.llvm.org/D45476



- F6099282: msg-9191-352.txt 


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44462: [clangd] Don't use DraftMgr in implementation of forceReparse.

[Mailing List &quot;cfe-commits&quot; via Phabricator via cfe-commits]

cfe-commits added a comment.



- F5892491: msg-4647-88.txt 


Repository:
  rL LLVM

https://reviews.llvm.org/D44462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44218: Correct the alignment for the PS4 target

[Mailing List &quot;cfe-commits&quot; via Phabricator via cfe-commits]

cfe-commits added a subscriber: erichkeane.
cfe-commits added a comment.

LGTM.

- F5880823: msg-22334-27.txt 


Repository:
  rC Clang

https://reviews.llvm.org/D44218



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits