[PATCH] D86993: Document Clang's expectations of the C standard library.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D86993#4655474 , @RalfJung wrote:

> With everything moving to github PRs, what are the next steps for this patch?

I'm unlikely to have cycles to work on this in the near future. If someone else 
wants to take over here, you would have my blessing :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86993/new/

https://reviews.llvm.org/D86993

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

[PATCH] D157879: [clang] Report missing designated initializers in C++

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This change has introduced a false positive for anonymous union members:

  struct A {
  int m;
  union { int n = 0; };
  };
  
  A a = A{.m = 0};

now produces a false positive warning saying that the anonymous union member in 
`A` is uninitialized.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157879/new/

https://reviews.llvm.org/D157879

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

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I've been pondering what I'd want from a warning here. I think generally I 
would like to warn if there are two plausible interpretations of the token 
sequence -- that is, if giving the `?` different precedence could plausibly 
lead to a different valid program. I think concretely that would lead to this 
rule:

Warn if: the condition in a conditional-expression has a suffix (right-hand 
operand, recursively, looking only through binary operators) that is plausibly 
a condition. That is:

- It is of a bool-like type: either `bool` itself, or an arithmetic or pointer 
or member pointer type, or a class with an `operator bool`.
- It is not a constant expression.

Compared to this patch, I think the main change would be the second bullet: do 
*not* warn if the potential alternative condition is a constant expression -- 
that's not a plausible condition for a conditional expression. Looking through 
the test changes in this patch, this change would remove the vast majority of 
the false positives where it's obvious to me as a reader of the code that the 
code was already correct, and just leave a few changes like the second one in 
cxa_personality.cpp where the code really does look ambiguous as written.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

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

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D147655#4650666 , @rjmccall wrote:

> Yeah, the more I think about this, the more I think that while (1) Apple 
> should upstream its use of an older default, regardless (2) the existence of 
> any targets at all with an older default means that tests like this always 
> need to be using `-fclang-abi-compat=latest`.

Seems reasonable. I went ahead and made that change in 
rG940850066290a484144db80f09e6c19709f5fe49 
.

I think we could probably limit this to just the test using 
`%itanium_abi_triple`, but there's also an affected test specifying 
`-triple=x86_64-apple-darwin9`, so that might not be enough for your needs. 
Also, I suppose some vendors might want to carry a patch that changes the 
default globally rather than on a per-target basis. So I just changed all the 
tests that touch the new mangling.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

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

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D147655#4649864 , @dyung wrote:

> Hi @rsmith, we have an internal test where your change seems to have changed 
> the mangling in C++17 mode and wanted to check if that was intentional.

[...]

> Are these changes in non-C++20 mode intentional?

Yes, they're intentional. Unfortunately we could have mangling collisions here 
when using the old ABI rule; these two different templates would have the same 
mangling:

  template  T returnit() {return I;};
  template  T returnit() { return I; }

But... it looks like this case is missing from the list of affected cases in 
the change description and the release notes. Sorry about that. Release notes 
updated in rGaaa79a59317f859485d701d1eb68ac4cb213e1d1 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

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

[PATCH] D153701: [Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

The changes in `SemaInit.cpp` don't look correct to me. Trying to recurse 
through the initializer and find every nested temporary is highly error-prone; 
I can't tell you which expression nodes you forgot to recurse through, but I'm 
sure there are some.

I think the approach being taken here is not really maintainable. We don't want 
the initialization code to need to know how to recurse through an expression 
after the fact and find all the temporaries that might need to be 
lifetime-extended, and we don't need it to do that either. Instead, we should 
make the expression evaluation context track the current for-range variable, 
and in `Sema::CreateMaterializeTemporaryExpr`, we should create a temporary 
that is already set to be lifetime-extended by the loop variable.




Comment at: clang/include/clang/Sema/Sema.h:1356-1357
 
+/// Whether rewrite the default argument.
+bool IsRewriteDefaultArgument = false;
+

Can you expand this comment to more fully describe what this flag governs? 
Which default argument? How would it be rewritten?



Comment at: clang/lib/Parse/ParseDecl.cpp:2330-2333
+  getLangOpts().CPlusPlus23);
+
+  // P2718R0 - Lifetime extension in range-based for loops.
+  if (getLangOpts().CPlusPlus23) {

cor3ntin wrote:
> We need to decide whether we want to backport thgis behavior to older 
> language mode, i think that would make sense.
> @hubert.reinterpretcast wdyt?
Definitely not. This is a visible behavior change to a rule that was not in any 
way incorrect or broken, and for which there were no implementation concerns. 
The change was not voted into the standard as a DR (see 
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/n4929.html). Applying 
this behavior in older standard versions would be a conformance bug.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153701/new/

https://reviews.llvm.org/D153701

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

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Ping. Are there any further concerns here? (This obviously needs to be merged 
with trunk, and the `-fclang-abi-compat=` checks and release notes need to be 
updated to match the latest release version.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

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

[PATCH] D158296: [clang] Diagnose overly complex Record in __builtin_dump_struct

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:732-733
+int RDKind = RD->isClass() ? 0 : (RD->isStruct() ? 1 : 2);
+S.Diag(PtrArg->getBeginLoc(), diag::err_builtin_dump_struct_too_complex)
+<< RDKind << RD->getName();
+return ExprError();

aaron.ballman wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > This will correctly handle diagnosing a gigantic anonymous struct.
> > Producing an error here seems likely to eventually cause problems in 
> > practice for some users: people are using `__builtin_dump_struct` in 
> > generic code for reflection purposes, not just for debugging, and this will 
> > cause us to start rejecting complex generic code.
> > 
> > Instead of rejecting, can we produce a tree of `PseudoObjectExpr`s if we 
> > have too many steps to store in a single expression?
> > Producing an error here seems likely to eventually cause problems in 
> > practice for some users: people are using __builtin_dump_struct in generic 
> > code for reflection purposes, not just for debugging, and this will cause 
> > us to start rejecting complex generic code.
> >
> > Instead of rejecting, can we produce a tree of PseudoObjectExprs if we have 
> > too many steps to store in a single expression?
> 
> I think that requires wider discussion -- I don't think 
> `__builtin_dump_struct` is a reasonable interface we want to support for 
> reflection (in fact, I'd argue it's an explicit non-goal, the same as 
> reflection via `-ast-dump`). Compile-time reflection is something we're 
> likely to need to support more intentionally and I don't think we're going to 
> want to use this as an interface for it or have to maintain it as a 
> reflection tool long-term. As such, I think producing a tree of 
> `PseudoObjectExpr`s is overkill; you can quote me on this a few years from 
> now when we're laughing at its quaintness, but "16k fields of debug output is 
> enough for anyone" for a debugging interface.
> 
> (That said, I think we should be considering what support we want to add to 
> the compiler for reflection in service of the work being done in WG21 on the 
> topic -- if `__builtin_dump_struct` is being used for reflection in practice, 
> it would be nice to give people a supported, more ergonomic interface for it 
> that we can use for a future version of C++.)
The bug report https://github.com/llvm/llvm-project/issues/63169 was 
encountered by a user hitting the previous 256-element limit in practice when 
using `__builtin_dump_struct` for reflection. I don't think we can reasonably 
prevent that from happening, other than -- as you say -- encouraging WG21 to 
give us a real reflection design we can implement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158296/new/

https://reviews.llvm.org/D158296

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

[PATCH] D158296: [clang] Diagnose overly complex Record in __builtin_dump_struct

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:732-733
+int RDKind = RD->isClass() ? 0 : (RD->isStruct() ? 1 : 2);
+S.Diag(PtrArg->getBeginLoc(), diag::err_builtin_dump_struct_too_complex)
+<< RDKind << RD->getName();
+return ExprError();

aaron.ballman wrote:
> This will correctly handle diagnosing a gigantic anonymous struct.
Producing an error here seems likely to eventually cause problems in practice 
for some users: people are using `__builtin_dump_struct` in generic code for 
reflection purposes, not just for debugging, and this will cause us to start 
rejecting complex generic code.

Instead of rejecting, can we produce a tree of `PseudoObjectExpr`s if we have 
too many steps to store in a single expression?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158296/new/

https://reviews.llvm.org/D158296

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

[PATCH] D158967: [clang][clangd] Ensure the stack bottom before building AST

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D158967#4621361 , @zyounan wrote:

>> Currently the code in CompilerInstance::ExecuteAction seems to acknowledge 
>> that there should be a fallback. I'm suggesting to move this fallback down 
>> to a function that actually runs parsing.
>
> One thing I'm afraid of is, that there are/were some compatible reasons that 
> made us decide not to insert this call at `ASTFrontendAction::ExecuteAction` 
> nor `clang::ParseAST`. (I didn't see the explanation in D66361 
> . Richard, could you kindly explain why? 
> @rsmith)

D66361  wasn't intended to cover all possible 
uses by itself. The recovery from deep recursion is best-effort, and it was 
expected that some tools that call into Clang in less common ways would need to 
manually call `noteBottomOfStack` if they wanted to enable this recovery. I put 
the fallback call in `CompilerInstance::ExecuteAction` because I thought it 
would do the right thing in most cases, and not require any effort for most 
tools. Adding another fallback in `ASTFrontendAction::ExecuteAction` seems OK 
to me if that's an entry point that's commonly used by tools. (Where is that 
being called from in clangd's case? Is there a higher point in the call stack 
where we could put the call to `noteBottomOfStack` instead?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158967/new/

https://reviews.llvm.org/D158967

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

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D155387#4579866 , @ilya-biryukov 
wrote:

> I believe this should compile as according to (over.match.oper)p4 
> :
>
>> A non-template function or function template F named operator== is a rewrite 
>> target with first operand o unless **a search for the name operator!= in the 
>> scope S** from the instantiation context of the operator expression finds a 
>> function …
>
> And 'search' is defined without any checks for ambiguous bases in 
> (basic.lookup.general)p3 
>
>> A single search in a scope S for a name N from a program point P finds all 
>> declarations that precede P to which any name that is the same as N 
>> ([basic.pre]) is bound in S. If any such declaration is a using-declarator 
>> whose terminal name ([expr.prim.id.unqual]) is not dependent 
>> ([temp.dep.type]), it is replaced by the declarations named by the 
>> using-declarator ([namespace.udecl]).
>
> Clang does a full lookup instead, therefore exposing the errors that it 
> should not diagnose.

A search of a class scope is ill-formed if it finds an ambiguous result, see 
http://eel.is/c++draft/basic.lookup#class.member.lookup-6.sentence-2 -- that's 
the rule that we're now enforcing more broadly, and it applies to all searches 
of class scopes. So I think Clang is correct to reject your testcase, even 
though the ambiguity seems much less relevant in this case and the rejection is 
surprising. The fix / workaround is to add `using X::operator!=; using 
Y::operator!=;` to the derived class.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155387/new/

https://reviews.llvm.org/D155387

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

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

The prior behavior of Clang is correct. A search of a class scope is ill-formed 
if it finds an ambiguous result, see 
http://eel.is/c++draft/basic.lookup#class.member.lookup-6.sentence-2


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157708/new/

https://reviews.llvm.org/D157708

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

[PATCH] D158372: [Clang] Treat invalid UDL as two tokens

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:97
 ^
-- Implemented `CWG1473 `_ which allows spaces after 
``operator""``.
-  Clang used to err on the lack of space when the literal suffix identifier 
was invalid in
-  all the language modes, which contradicted the deprecation of the 
whitespaces.
-  Also turn on ``-Wdeprecated-literal-operator`` by default in all the 
language modes.
+- Implemented `CWG1473 `_ allowing lack of space 
after ``operator""``.
+  Clang used to err on the lack of space when the literal suffix identifier 
was invalid,

I don't think this is accurate. Clang supported CWG1473 before these changes, 
as far as I can see: all valid code under CWG1473 was accepted, and invalid 
code was diagnosed (by default). Rather, what has changed is the behavior for 
invalid code: instead of treating an invalid `""blah` as two tokens always, in 
order to accept as much old code as possible, we now treat it as two tokens 
only when `blah` is defined as a macro name.

This is still a breaking change in some cases, for users of 
`-Wno-deprecated-literal-operator`, eg:

```
#define FOO(xyz) ""xyz
```

...  now will be lexed as a single invalid token rather than two tokens.

I'm not sure what the motivation for making changes here was, and D153156's 
description doesn't really help me understand it. Is the goal to improve the 
diagnostic quality for these kinds of errors on invalid code? Is there some 
example for which Clang's behavior with regard to CWG1473 was non-conforming? 
Something else?



Comment at: clang/lib/Lex/Lexer.cpp:2020
+  bool IsLiteralOperator =
+  StringRef(BufferPtr, 2).equals("\"\"") && BufferPtr + 2 == TokStart;
+  if (unsigned TokLen = CurPtr - TokStart;

Reverse the order of these checks to do the cheaper check first and to avoid 
the possibility of reading off the end of the input.



Comment at: clang/lib/Lex/Lexer.cpp:2027-2029
+// However, don't diagnose operator""E(...) even if E is a macro as it
+// results in confusing error messages. Hence, ""E would not be treated as
+// string concat; instead it's a single PP token (as it should be).

That's still a breaking change compared to what we designed 
`-Wno-reserved-literal-operator` to do. How often does it happen in practice 
that someone has both an ill-formed literal operator *and* a macro defined to 
the same name as the suffix?



Comment at: clang/lib/Lex/Lexer.cpp:2035
+IdentifierInfo *II = PP->LookUpIdentifierInfo(Result);
+if (II->hasMacroDefinition()) {
+  Diag(TokStart, LangOpts.MSVCCompat

This doesn't check whether the identifier is currently defined as a macro, in 
the presence of modules. It also won't be correct if the lexer has got 
substantially ahead of the preprocessor, and the `#define` has been lexed but 
not yet preprocessed.

Overall, it's not really possible to tell from here whether an identifier is 
defined as a macro. To do this properly, you should instead produce a single 
token here and teach the preprocessor to consider splitting it into two tokens 
if the suffix is a reserved ud-suffix naming a defined macro. In principle you 
could also check from the preprocessor whether the previous produced token was 
`operator` and use that as part of the decision...



Comment at: clang/test/CXX/drs/dr14xx.cpp:487
 
 namespace dr1473 { // dr1473: 18
// NB: sup 1762, test reused there

I don't think this is correct. As far as I can tell, Clang has correctly 
implemented CWG1473 since version 3.2.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158372/new/

https://reviews.llvm.org/D158372

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

[PATCH] D154559: [clang] Fix constant evaluation about static member function

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

Clang was correct here until fairly recently; P2280 
 
(https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2280r4.html) changed 
the language rules. It was applied as a DR, so we should make that change 
retroactively rather than only in C++23 mode. See 
https://github.com/llvm/llvm-project/issues/63139, which tracks implementation 
of that language change.

I don't think we should be applying an ad-hoc patch like this which doesn't 
follow either the old or the new language rule, and should instead implement 
the new rule from P2280 .


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154559/new/

https://reviews.llvm.org/D154559

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:14254
 int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified()) {
-  if (HasConstInit)

efriedma wrote:
> rnk wrote:
> > I think this is not compatible with MSVC. MSVC uses simple logic, it 
> > doesn't look for mutable: https://gcc.godbolt.org/z/sj6d4saxx
> > 
> > The const mutable struct appears in the myrdata section in that example.
> > 
> > I think the solution is to separate the flag logic from the pragma stack 
> > selection logic, which has to remain MSVC-compatible.
> MSVC apparently looks at whether the variable is marked "const", and nothing 
> else; it doesn't look at mutable, it doesn't look at whether the variable has 
> a constant initializer.  So the current code isn't right either; if we're 
> trying to implement MSVC-compatible logic, we shouldn't check HasConstInit.
> 
> That said, I'm not sure how precisely/in what modes we want to precisely 
> emulate MSVC.  Probably anything we do here will be confusing.
We should at least issue a warning if the sensible logic and the 
MSVC-compatible calculation differ. @rnk, do you know how important it is to 
follow the MSVC semantics in this regard?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

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

[PATCH] D156806: [Modules] Add test for merging of template member parent

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Awesome, thanks! It makes perfect sense that rG61c7a9140b 
 would fix 
this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156806/new/

https://reviews.llvm.org/D156806

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

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

This looks good to me. I think we can further refine the handling of duplicate 
diagnostics but I don't think that needs to be done as part of this bugfix.




Comment at: clang/lib/Sema/SemaOverload.cpp:953
 S.LookupQualifiedName(Members, RHSRec->getDecl());
-Members.suppressDiagnostics();
+Members.suppressAccessDiagnostics();
 for (NamedDecl *Op : Members)

Can we get duplicate diagnostics here if this lookup fails due to ambiguity? I 
think we'll perform the same lookup again when looking for candidates if the 
original operator was `!=`.

(I wonder if we can fix that by avoiding the duplicate lookup, rather than by 
suppressing ambiguity diagnostics.)



Comment at: clang/lib/Sema/SemaOverload.cpp:964
S.getScopeForContext(EqFD->getEnclosingNamespaceContext()));
-  NonMembers.suppressDiagnostics();
+  NonMembers.suppressAccessDiagnostics();
   for (NamedDecl *Op : NonMembers) {

There can't be any access diagnostics here because this is a namespace-scope 
lookup. You can just drop this call entirly.



Comment at: clang/lib/Sema/SemaOverload.cpp:15083
   case OR_Ambiguous:
-CandidateSet.NoteCandidates(
-PartialDiagnosticAt(Object.get()->getBeginLoc(),
-PDiag(diag::err_ovl_ambiguous_object_call)
-<< Object.get()->getType()
-<< Object.get()->getSourceRange()),
-*this, OCD_AmbiguousCandidates, Args);
+if (!R.isAmbiguous())
+  CandidateSet.NoteCandidates(

I think we should also check whether the ambiguous viable functions were all 
first declared in the same base class here. If so, then it still makes sense to 
`NoteCandidates`, because the ambiguous lookup had nothing to do with the 
ambiguous overload resolution.



Comment at: clang/lib/Sema/SemaOverload.cpp:15293
   case OR_Ambiguous:
-CandidateSet.NoteCandidates(
-PartialDiagnosticAt(OpLoc, PDiag(diag::err_ovl_ambiguous_oper_unary)
-   << "->" << Base->getType()
-   << Base->getSourceRange()),
-*this, OCD_AmbiguousCandidates, Base);
+if (!R.isAmbiguous())
+  CandidateSet.NoteCandidates(

(Likewise here.)



Comment at: clang/lib/Sema/SemaOverload.cpp:15191
   OverloadCandidateSet::iterator Best;
   switch (CandidateSet.BestViableFunction(*this, OpLoc, Best)) {
   case OR_Success:

shafik wrote:
> rsmith wrote:
> > shafik wrote:
> > > @rsmith if `R.isAmbiguous()` should we even check the overload 
> > > candidates? 
> > > 
> > > Right now in `clang/test/SemaCXX/arrow-operator.cpp` we are getting both 
> > > an ambiguous name lookup and overload diagnostic. 
> > Hm, I think perhaps the ideal thing to do is: if `BestViableFunction` 
> > returns `OR_Ambiguous`, the lookup was ambiguous, and we found viable 
> > functions in multiple different base classes, then don't produce any 
> > further diagnostics. That way, we still get good error recovery in the case 
> > where the overloading is unambiguous despite the lookup being ambiguous, or 
> > when the overloading is ambiguous for some reason unrelated to the 
> > ambiguous lookup.
> > 
> > (We'd want to do this in all the overloaded operator lookups in this file, 
> > not just for `operator->`.)
> I only found one other location that matched this pattern if you meant a 
> larger change let me know. 
Ideally, we should make `AddMemberOperatorCandidates` (and 
`LookupOverloadedBinOp`) inform its three callers whether its lookup was 
ambiguous, so that they can also suppress the duplicate ambiguity diagnostics, 
for unary operators, binary operators, and subscripting.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155387/new/

https://reviews.llvm.org/D155387

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

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

LGTM, though please wait a day or two for any more comments from @gribozavr2 
since he's looked at this more closely than I have.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155895/new/

https://reviews.llvm.org/D155895

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

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10339-10343
+  if (RT->getDecl()->isAnonymousStructOrUnion()) {
+FieldsToCheck.append(RT->getDecl()->field_begin(),
+ RT->getDecl()->field_end());
+continue;
+  }

Please add a check for `ClangABICompat` here (only do the new thing if 
`getLangOpts().getClangABICompat() > LangOptions::ClangABI::Ver17`) so that 
people with a stable ABI requirement can use `-fclang-abi-compat=17.0` or 
earlier to turn off this ABI change. This is the first ABI change since we 
branched off the 17.x release, so you'll also need to add the new `Ver17` 
enumerator and parsing support for it; grep for `Ver15` to find the places you 
may need to update.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155895/new/

https://reviews.llvm.org/D155895

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D154658#4533476 , @MatzeB wrote:

> This change results in some of our builds (distributed Thin-LTO in case that 
> matters) to fail with missing symbols. At a first glance this seems to emit 
> VTables in some files where it didn't do this before and then fails to 
> resolve some members of that vtable. I'm in the process of analyzing this 
> further and making a small reproducer.

Please try again after rGb6847edfc235829b37dd6d734ef5bbfa0a58b6fc 
 and see 
if you're still having issues.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9d525bf94b25: Optimize emission of `dynamic_cast` to final 
classes. (authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D154658?vs=542693=543133#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/AST/ExprCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGenCXX/dynamic-cast-always-null.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact.cpp
  clang/test/Driver/fassume-unique-vtables.cpp

Index: clang/test/Driver/fassume-unique-vtables.cpp
===
--- /dev/null
+++ clang/test/Driver/fassume-unique-vtables.cpp
@@ -0,0 +1,4 @@
+// RUN: %clang -### -fno-assume-unique-vtables %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -fno-assume-unique-vtables -fassume-unique-vtables %s -S 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-fno-assume-unique-vtables"
+// CHECK-NOOPT-NOT: "-fno-assume-unique-vtables"
Index: clang/test/CodeGenCXX/dynamic-cast-exact.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dynamic-cast-exact.cpp
@@ -0,0 +1,78 @@
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions -std=c++11 -o - | FileCheck %s --implicit-check-not='call {{.*}} @__dynamic_cast'
+struct Offset { virtual ~Offset(); };
+struct A { virtual ~A(); };
+struct B final : Offset, A { };
+
+struct C { virtual ~C(); int c; };
+struct D : A { int d; };
+struct E : A { int e; };
+struct F : virtual A { int f; };
+struct G : virtual A { int g; };
+struct H final : C, D, E, F, G { int h; };
+
+// CHECK-LABEL: @_Z7inexactP1A
+C *inexact(A *a) {
+  // CHECK: call {{.*}} @__dynamic_cast
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z12exact_singleP1A
+B *exact_single(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z9exact_refR1A
+B _ref(A ) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: call {{.*}} @__cxa_bad_cast
+  // CHECK: unreachable
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: ret ptr %[[RESULT]]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z11exact_multiP1A
+H *exact_multi(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[OFFSET_TO_TOP_SLOT:.*]] = getelementptr inbounds i64, ptr %[[VPTR]], i64 -2
+  // CHECK: %[[OFFSET_TO_TOP:.*]] = load i64, ptr %[[OFFSET_TO_TOP_SLOT]]
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 %[[OFFSET_TO_TOP]]
+  // CHECK: %[[DERIVED_VPTR:.*]] = load ptr, ptr %[[RESULT]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[DERIVED_VPTR]], getelementptr inbounds ({ [5 x ptr], [4 x ptr], [4 x ptr], [6 x ptr], [6 x ptr] }, ptr @_ZTV1H, i32 0, inrange i32 0, i32 3)
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return 

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D154324#4524235 , @v.g.vassilev 
wrote:

> @rsmith, thanks for the suggestions! Could you go over 
> `ODRHash::AddTemplateName` suggest how to fix it to address 
> https://reviews.llvm.org/D153003 and https://reviews.llvm.org/D41416#4496451?

`AddTemplateName` looks fine as-is to me; I think the problem in D153003 
 is that we'd stepped outside of the entity 
we were odr-hashing and started hashing something else, which (legitimately) 
was different between translation units.

For D41416 , ODR hashing may not be the best 
mechanism to hash the template arguments, unfortunately. ODR hashing is (or 
perhaps, should be) about determining whether two things are spelled the same 
way and have the same meaning (as required by the C++ ODR), whereas I think 
what you're looking for is whether they have the same meaning regardless of 
spelling. Maybe we can get away with reusing ODR hashing anyway, on the basis 
that any canonical, non-dependent template argument should have the same 
(invented) spelling in every translation unit, but I'm not certain that's true 
in all cases. There may still be cases where the canonical type includes some 
aspect of "whatever we saw first", in which case the ODR hash can differ across 
translation units for non-dependent, canonical template arguments that are 
spelled differently but have the same meaning, though I can't think of one 
off-hand.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154324/new/

https://reviews.llvm.org/D154324

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

[PATCH] D156000: Track the RequestingModule in the HeaderSearch LookupFile cache.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7c5e4efb099e: Track the RequestingModule in the HeaderSearch 
LookupFile cache. (authored by rsmith).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156000/new/

https://reviews.llvm.org/D156000

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/lookup-file-cache.cpp


Index: clang/test/Modules/lookup-file-cache.cpp
===
--- /dev/null
+++ clang/test/Modules/lookup-file-cache.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -I %t/include -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -fsyntax-only %t/tu.c
+
+//--- include/module.modulemap
+module A [no_undeclared_includes] { textual header "A.h" }
+module B { header "B.h" }
+
+//--- include/A.h
+#if __has_include()
+#error B.h should not be available from A.h.
+#endif
+
+//--- include/B.h
+// This file intentionally left blank.
+
+//--- tu.c
+#if !__has_include()
+#error B.h should be available from tu.c.
+#endif
+
+#include "A.h"
+
+#if !__has_include()
+#error B.h should still be available from tu.c.
+#endif
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1008,7 +1008,8 @@
   ConstSearchDirIterator NextIt = std::next(It);
 
   if (!SkipCache) {
-if (CacheLookup.StartIt == NextIt) {
+if (CacheLookup.StartIt == NextIt &&
+CacheLookup.RequestingModule == RequestingModule) {
   // HIT: Skip querying potentially lots of directories for this lookup.
   if (CacheLookup.HitIt)
 It = CacheLookup.HitIt;
@@ -1021,7 +1022,7 @@
   // MISS: This is the first query, or the previous query didn't match
   // our search start.  We will fill in our found location below, so prime
   // the start point value.
-  CacheLookup.reset(/*NewStartIt=*/NextIt);
+  CacheLookup.reset(RequestingModule, /*NewStartIt=*/NextIt);
 
   if (It == search_dir_begin() && FirstNonHeaderMapSearchDirIdx > 0) {
 // Handle cold misses of user includes in the presence of many header
@@ -1036,8 +1037,9 @@
   It = search_dir_nth(Iter->second);
   }
 }
-  } else
-CacheLookup.reset(/*NewStartIt=*/NextIt);
+  } else {
+CacheLookup.reset(RequestingModule, /*NewStartIt=*/NextIt);
+  }
 
   SmallString<64> MappedName;
 
Index: clang/include/clang/Lex/HeaderSearch.h
===
--- clang/include/clang/Lex/HeaderSearch.h
+++ clang/include/clang/Lex/HeaderSearch.h
@@ -277,6 +277,9 @@
 
   /// Keeps track of each lookup performed by LookupFile.
   struct LookupFileCacheInfo {
+// The requesting module for the lookup we cached.
+const Module *RequestingModule = nullptr;
+
 /// Starting search directory iterator that the cached search was performed
 /// from. If there is a hit and this value doesn't match the current query,
 /// the cache has to be ignored.
@@ -292,7 +295,9 @@
 /// Default constructor -- Initialize all members with zero.
 LookupFileCacheInfo() = default;
 
-void reset(ConstSearchDirIterator NewStartIt) {
+void reset(const Module *NewRequestingModule,
+   ConstSearchDirIterator NewStartIt) {
+  RequestingModule = NewRequestingModule;
   StartIt = NewStartIt;
   MappedName = nullptr;
 }


Index: clang/test/Modules/lookup-file-cache.cpp
===
--- /dev/null
+++ clang/test/Modules/lookup-file-cache.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -I %t/include -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -fsyntax-only %t/tu.c
+
+//--- include/module.modulemap
+module A [no_undeclared_includes] { textual header "A.h" }
+module B { header "B.h" }
+
+//--- include/A.h
+#if __has_include()
+#error B.h should not be available from A.h.
+#endif
+
+//--- include/B.h
+// This file intentionally left blank.
+
+//--- tu.c
+#if !__has_include()
+#error B.h should be available from tu.c.
+#endif
+
+#include "A.h"
+
+#if !__has_include()
+#error B.h should still be available from tu.c.
+#endif
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1008,7 +1008,8 @@
   ConstSearchDirIterator NextIt = std::next(It);
 
   if (!SkipCache) {
-if (CacheLookup.StartIt == NextIt) {
+if (CacheLookup.StartIt == NextIt &&
+CacheLookup.RequestingModule == RequestingModule) {
   // HIT: Skip querying potentially lots of directories for this lookup.
   

[PATCH] D153701: [WIP][Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914
+  // [P2718R0] Lifetime extension in range-based for loops.
+  //
+  // 6.7.7 [class.temporary] p5:
+  // There are four contexts in which temporaries are destroyed at a different
+  // point than the end of the full-expression.
+  //
+  // 6.7.7 [class.temporary] p6:

hubert.reinterpretcast wrote:
> yronglin wrote:
> > hubert.reinterpretcast wrote:
> > > yronglin wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > yronglin wrote:
> > > > > > yronglin wrote:
> > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > yronglin wrote:
> > > > > > > > > rsmith wrote:
> > > > > > > > > > This isn't the right way to model the behavior here -- the 
> > > > > > > > > > presence or absence of an `ExprWithCleanups` is just a 
> > > > > > > > > > convenience to tell consumers of the AST whether they 
> > > > > > > > > > should expect to see cleanups later or not, and doesn't 
> > > > > > > > > > carry an implication of affecting the actual temporary 
> > > > > > > > > > lifetimes and storage durations.
> > > > > > > > > > 
> > > > > > > > > > The outcome that we should be aiming to reach is that all 
> > > > > > > > > > `MaterializeTemporaryExpr`s created as part of processing 
> > > > > > > > > > the for-range-initializer are marked as being 
> > > > > > > > > > lifetime-extended by the for-range variable. Probably the 
> > > > > > > > > > simplest way to handle that would be to track the current 
> > > > > > > > > > enclosing for-range-initializer variable in the 
> > > > > > > > > > `ExpressionEvaluationContextRecord`, and whenever a 
> > > > > > > > > > `MaterializeTemporaryExpr` is created, if there is a 
> > > > > > > > > > current enclosing for-range-initializer, mark that 
> > > > > > > > > > `MaterializeTemporaryExpr` as being lifetime-extended by it.
> > > > > > > > > Awesome! Thanks a lot for your advice, this is very helpful! 
> > > > > > > > > I want to take a longer look at it.
> > > > > > > > As mentioned in D139586, `CXXDefaultArgExpr`s may need 
> > > > > > > > additional handling. Similarly for `CXXDefaultInitExpr`s.
> > > > > > > Thanks for your tips! I have a question that what's the correct 
> > > > > > > way to extent the lifetime of `CXXBindTemporaryExpr`? Can I just 
> > > > > > > `materialize` the temporary? It may replaced by 
> > > > > > > `MaterializeTemporaryExpr`, and then I can mark it as being 
> > > > > > > lifetime-extended by the for-range variable.
> > > > > > Eg.
> > > > > > ```
> > > > > > void f() {
> > > > > >   int v[] = {42, 17, 13};
> > > > > >   Mutex m;
> > > > > >   for (int x : static_cast(LockGuard(m)), v) // lock released 
> > > > > > in C++ 2020
> > > > > >   {
> > > > > > LockGuard guard(m); // OK in C++ 2020, now deadlocks
> > > > > >   }
> > > > > > }
> > > > > > ```
> > > > > > ```
> > > > > > BinaryOperator 0x135036220 'int[3]' lvalue ','
> > > > > > |-CXXStaticCastExpr 0x1350361d0 'void' static_cast 
> > > > > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct 
> > > > > > LockGuard' functional cast to LockGuard 
> > > > > > |   `-CXXBindTemporaryExpr 0x135036178 'LockGuard':'struct 
> > > > > > LockGuard' (CXXTemporary 0x135036178)
> > > > > > | `-CXXConstructExpr 0x135036140 'LockGuard':'struct LockGuard' 
> > > > > > 'void (Mutex &)'
> > > > > > |   `-DeclRefExpr 0x135035e18 'Mutex':'class Mutex' lvalue Var 
> > > > > > 0x135035b40 'm' 'Mutex':'class Mutex'
> > > > > > `-DeclRefExpr 0x135036200 'int[3]' lvalue Var 0x135035928 'v' 
> > > > > > 'int[3]'
> > > > > > ```
> > > > > If `MaterializeTemporaryExpr` represents a "temporary materialization 
> > > > > conversion", then the above should already have one just under the 
> > > > > `static_cast` to `void` (since the cast operand would be a 
> > > > > discarded-value expression).
> > > > > 
> > > > > There may be unfortunate effects from materializing temporaries for 
> > > > > discarded-value expressions though: Technically, temporaries are also 
> > > > > created for objects having scalar type.
> > > > > 
> > > > > Currently, `MaterializeTemporaryExpr` is documented as being tied to 
> > > > > reference binding, but that is not correct: for example, 
> > > > > `MaterializeTemporaryExpr` also appears when a member access is made 
> > > > > on a temporary of class type.
> > > > http://eel.is/c++draft/class.temporary says:
> > > > ```
> > > > [Note 3: Temporary objects are materialized:
> > > > ...
> > > > (2.6)
> > > > when a prvalue that has type other than cv void appears as a 
> > > > discarded-value expression ([expr.context]).
> > > > — end note]
> > > > ```
> > > > Seems we should materialized the discard-value expression in this case, 
> > > > WDYT?
> > > I think we should, but what is the codegen fallout? Would no-opt builds 
> > > start writing `42` into allocated memory for `static_cast(42)`?
> > Thanks for your confirm @hubert.reinterpretcast ! 
> > 
> 

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I've done a pass through this file looking for places where we incorrectly add 
to the ODR hash a type that was written within some other entity than the one 
that we're ODR hashing, that could validly be spelled differently in different 
declarations of that other entity. There are quite a lot of them; please see 
the comments here for places that need fixing.

Ideally, we should only be hashing a type when we have a corresponding 
`TypeSourceInfo` (that describes how that type was written in the source code) 
and hence a `TypeLoc`. Similarly, for template arguments, we'd like to have a 
`TemplateArguentLoc` instead of a `TemplateArgument`. So if you want to handle 
this properly, the best thing would be to change this code so that it can only 
hash `TemplateArgumentLoc`s and `TypeLoc`s, not `TemplateArgument`s and 
`QualType`s, but that would be a substantial amount of work; just changing it 
so we start with a type-as-written should be good enough to get it working 
properly.




Comment at: clang/lib/AST/ODRHash.cpp:297-302
   void VisitValueDecl(const ValueDecl *D) {
 if (!isa(D)) {
   AddQualType(D->getType());
 }
 Inherited::VisitValueDecl(D);
   }

For a `DeclaratorDecl` we should be adding `D->getTypeSourceInfo()->getType()` 
(the type as written), not `D->getType()` (the resolved type); for a 
`ValueDecl` that is not a `DeclaratorDecl`, we shouldn't include the type at 
all, because it wasn't written in the source code.



Comment at: clang/lib/AST/ODRHash.cpp:354
 ID.AddInteger(D->getPropertyImplementation());
 AddQualType(D->getType());
 AddDecl(D);





Comment at: clang/lib/AST/ODRHash.cpp:397
 
 AddQualType(Method->getReturnType());
 ID.AddInteger(Method->param_size());





Comment at: clang/lib/AST/ODRHash.cpp:862-910
   // Return the RecordType if the typedef only strips away a keyword.
   // Otherwise, return the original type.
   static const Type *RemoveTypedef(const Type *T) {
 const auto *TypedefT = dyn_cast(T);
 if (!TypedefT) {
   return T;
 }

We should not be stripping typedefs here!



Comment at: clang/lib/AST/ODRHash.cpp:915-939
 QualType Original = T->getOriginalType();
 QualType Adjusted = T->getAdjustedType();
 
 // The original type and pointee type can be the same, as in the case of
 // function pointers decaying to themselves.  Set a bool and only process
 // the type once, to prevent doubling the work.
 SplitQualType split = Adjusted.split();

We should only be hashing the type that was written in the source code here, 
not the adjusted type that's computed from it (and might partially desugar that 
original type).



Comment at: clang/lib/AST/ODRHash.cpp:976
 AddQualType(T->getModifiedType());
 AddQualType(T->getEquivalentType());
 

We should defensively not include the equivalent type here, because it wasn't 
written in the entity that we're ODR hashing and might in principle depend on 
the spelling of a type elsewhere (depending on what the attribute does). The 
modified type is written in the source so it's fine to include it.



Comment at: clang/lib/AST/ODRHash.cpp:998
 AddStmt(T->getUnderlyingExpr());
 AddQualType(T->getUnderlyingType());
 VisitType(T);

We should not hash this, because it can differ between identical types that are 
written the same way.



Comment at: clang/lib/AST/ODRHash.cpp:1187-1203
 QualType UnderlyingType = T->getDecl()->getUnderlyingType();
 VisitQualifiers(UnderlyingType.getQualifiers());
 while (true) {
   if (const TypedefType *Underlying =
   dyn_cast(UnderlyingType.getTypePtr())) {
 UnderlyingType = Underlying->getDecl()->getUnderlyingType();
 continue;

This code is also wrong, and looks like the root cause of the issue here. We 
shouldn't be including the underlying type of a typedef type in the ODR hash, 
because it can be written differently in different declarations of the typedef 
declaration.

If we want to include the underlying type of the typedef here, we'll need a new 
kind of hashing to capture only the value of the typedef and not how it was 
written. I don't think it's worth it; let's just not include the definition of 
a referenced typedef in the hash for now.



Comment at: clang/lib/AST/ODRHash.cpp:1210-1211
 Hash.AddBoolean(T->isSugared());
 if (T->isSugared())
   AddQualType(T->desugar());
 

We should also not hash this, because it can differ between identical types.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154324/new/

https://reviews.llvm.org/D154324

___

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:15191
   OverloadCandidateSet::iterator Best;
   switch (CandidateSet.BestViableFunction(*this, OpLoc, Best)) {
   case OR_Success:

shafik wrote:
> @rsmith if `R.isAmbiguous()` should we even check the overload candidates? 
> 
> Right now in `clang/test/SemaCXX/arrow-operator.cpp` we are getting both an 
> ambiguous name lookup and overload diagnostic. 
Hm, I think perhaps the ideal thing to do is: if `BestViableFunction` returns 
`OR_Ambiguous`, the lookup was ambiguous, and we found viable functions in 
multiple different base classes, then don't produce any further diagnostics. 
That way, we still get good error recovery in the case where the overloading is 
unambiguous despite the lookup being ambiguous, or when the overloading is 
ambiguous for some reason unrelated to the ambiguous lookup.

(We'd want to do this in all the overloaded operator lookups in this file, not 
just for `operator->`.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155387/new/

https://reviews.llvm.org/D155387

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

[PATCH] D156000: Track the RequestingModule in the HeaderSearch LookupFile cache.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added a reviewer: jansvoboda11.
Herald added a project: All.
rsmith requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Different requesting modules can have different lookup results, so don't
cache results across modules.

Fixes a regression introduced in reviews.llvm.org/D132779 
.

Test case based on one provided by Jan Svoboda.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156000

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/lookup-file-cache.cpp


Index: clang/test/Modules/lookup-file-cache.cpp
===
--- /dev/null
+++ clang/test/Modules/lookup-file-cache.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -I %t/include -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -fsyntax-only %t/tu.c
+
+//--- include/module.modulemap
+module A [no_undeclared_includes] { textual header "A.h" }
+module B { header "B.h" }
+
+//--- include/A.h
+#if __has_include()
+#error B.h should not be available from A.h.
+#endif
+
+//--- include/B.h
+// This file intentionally left blank.
+
+//--- tu.c
+#if !__has_include()
+#error B.h should be available from tu.c.
+#endif
+
+#include "A.h"
+
+#if !__has_include()
+#error B.h should still be available from tu.c.
+#endif
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1008,7 +1008,8 @@
   ConstSearchDirIterator NextIt = std::next(It);
 
   if (!SkipCache) {
-if (CacheLookup.StartIt == NextIt) {
+if (CacheLookup.StartIt == NextIt &&
+CacheLookup.RequestingModule == RequestingModule) {
   // HIT: Skip querying potentially lots of directories for this lookup.
   if (CacheLookup.HitIt)
 It = CacheLookup.HitIt;
@@ -1021,7 +1022,7 @@
   // MISS: This is the first query, or the previous query didn't match
   // our search start.  We will fill in our found location below, so prime
   // the start point value.
-  CacheLookup.reset(/*NewStartIt=*/NextIt);
+  CacheLookup.reset(RequestingModule, /*NewStartIt=*/NextIt);
 
   if (It == search_dir_begin() && FirstNonHeaderMapSearchDirIdx > 0) {
 // Handle cold misses of user includes in the presence of many header
@@ -1036,8 +1037,9 @@
   It = search_dir_nth(Iter->second);
   }
 }
-  } else
-CacheLookup.reset(/*NewStartIt=*/NextIt);
+  } else {
+CacheLookup.reset(RequestingModule, /*NewStartIt=*/NextIt);
+  }
 
   SmallString<64> MappedName;
 
Index: clang/include/clang/Lex/HeaderSearch.h
===
--- clang/include/clang/Lex/HeaderSearch.h
+++ clang/include/clang/Lex/HeaderSearch.h
@@ -277,6 +277,9 @@
 
   /// Keeps track of each lookup performed by LookupFile.
   struct LookupFileCacheInfo {
+// The requesting module for the lookup we cached.
+const Module *RequestingModule = nullptr;
+
 /// Starting search directory iterator that the cached search was performed
 /// from. If there is a hit and this value doesn't match the current query,
 /// the cache has to be ignored.
@@ -292,7 +295,9 @@
 /// Default constructor -- Initialize all members with zero.
 LookupFileCacheInfo() = default;
 
-void reset(ConstSearchDirIterator NewStartIt) {
+void reset(const Module *NewRequestingModule,
+   ConstSearchDirIterator NewStartIt) {
+  RequestingModule = NewRequestingModule;
   StartIt = NewStartIt;
   MappedName = nullptr;
 }


Index: clang/test/Modules/lookup-file-cache.cpp
===
--- /dev/null
+++ clang/test/Modules/lookup-file-cache.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -I %t/include -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -fsyntax-only %t/tu.c
+
+//--- include/module.modulemap
+module A [no_undeclared_includes] { textual header "A.h" }
+module B { header "B.h" }
+
+//--- include/A.h
+#if __has_include()
+#error B.h should not be available from A.h.
+#endif
+
+//--- include/B.h
+// This file intentionally left blank.
+
+//--- tu.c
+#if !__has_include()
+#error B.h should be available from tu.c.
+#endif
+
+#include "A.h"
+
+#if !__has_include()
+#error B.h should still be available from tu.c.
+#endif
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1008,7 +1008,8 @@
   ConstSearchDirIterator NextIt = std::next(It);
 
   if (!SkipCache) {
-if (CacheLookup.StartIt == NextIt) {
+if (CacheLookup.StartIt == NextIt &&
+  

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D132779#4523783 , @jansvoboda11 
wrote:

> Hi @rsmith, this commit makes it possible for `HeaderInfo::LookupFile()` to 
> be called with different `RequestingModule` within single `CompilerInstance`. 
> This is problematic, since some modules may see headers other modules can't 
> (due to `[no_undeclared_includes]`). This can permanently mess up contents of 
> the lookup cache (`HeaderSearch::LookupFileCache`) that uses only the lookup 
> name as the key.

Hm, interesting. I wonder if this was (subtly) broken before, given that it was 
previously possible to call `LookupFile` with different `RequestingModule`s in 
a single compilation -- once with a null `RequestingModule` and once with the 
compilation's current module. In any case, presumably we should either ensure 
that the cached data is independent of the requesting module (eg, apply the 
decluse restriction after computing the iterator pair), or add the requesting 
module to the key, or only cache data for one specific value of 
`RequestingModule`. Maybe the simplest thing would be to use a single-item LRU 
cache here -- store the `RequestingModule` in the cached result and ignore the 
existing cached result if it doesn't match the current `RequestingModule`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132779/new/

https://reviews.llvm.org/D132779

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

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

OK, I see. The problem is that the canonical version of the type can be spelled 
in different ways in different translation units, due to us treating some 
expressions as being equivalent despite them not being the same under the ODR. 
For example, we consider these function template declarations to be 
redeclarations:

  namespace N {
int x;
template void f(decltype(T(x)));
  }
  template void f(decltype(T(::N::x))) {}

... but the ODR considers the expressions `x` and `::N::x` to be distinct. That 
means that the canonical form of the type "`decltype` of 
`N::x`-cast-to-``" has two possible different 
spellings. So we can't use the approach of mapping to the canonical type before 
forming an ODR hash -- doing so is not correct.

Instead, let's use the other approach that I suggested, and add the spelling of 
the base class specifier (the type in its `TypeSourceInfo`) to the ODR hash 
instead of its canonical type.




Comment at: clang/lib/AST/ODRHash.cpp:596
   for (const auto  : Bases) {
-AddQualType(Base.getType());
+AddQualType(Base.getType().getCanonicalType());
 ID.AddInteger(Base.isVirtual());

Let's hash the type-as-written instead of hashing the canonical type. 
(`Base.getType()` gets the unqualified version of the type, which can partially 
desugar it, and can lead to different representations in different TUs.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154324/new/

https://reviews.llvm.org/D154324

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

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This looks correct to me, but it's still a little subtle. Perhaps it'd be 
clearer to map the method to an integer (0 for copy assignment, 1 for move 
assignment, 2 for destructor, 3 for equality comparison), and then order them 
by that integer? That'd be more obviously a strict weak order.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D154658#4516554 , @rjmccall wrote:

> LGTM, except, should we have a way to turn this optimization off specifically?

Sure. I suppose even for an internal linkage vtable there could be a reason to 
want to turn this off (eg, if someone constructs a custom vtable at runtime). 
I've added `-fno-assume-unique-vtables` to disable the optimization.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 542693.
rsmith added a comment.
Herald added a subscriber: MaskRay.

- Add option to disable vptr-based dynamic_cast optimization.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/AST/ExprCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGenCXX/dynamic-cast-always-null.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact.cpp
  clang/test/Driver/fassume-unique-vtables.cpp

Index: clang/test/Driver/fassume-unique-vtables.cpp
===
--- /dev/null
+++ clang/test/Driver/fassume-unique-vtables.cpp
@@ -0,0 +1,4 @@
+// RUN: %clang -### -fno-assume-unique-vtables %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -fno-assume-unique-vtables -fassume-unique-vtables %s -S 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-fno-assume-unique-vtables"
+// CHECK-NOOPT-NOT: "-fno-assume-unique-vtables"
Index: clang/test/CodeGenCXX/dynamic-cast-exact.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dynamic-cast-exact.cpp
@@ -0,0 +1,78 @@
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions -std=c++11 -o - | FileCheck %s --implicit-check-not='call {{.*}} @__dynamic_cast'
+struct Offset { virtual ~Offset(); };
+struct A { virtual ~A(); };
+struct B final : Offset, A { };
+
+struct C { virtual ~C(); int c; };
+struct D : A { int d; };
+struct E : A { int e; };
+struct F : virtual A { int f; };
+struct G : virtual A { int g; };
+struct H final : C, D, E, F, G { int h; };
+
+// CHECK-LABEL: @_Z7inexactP1A
+C *inexact(A *a) {
+  // CHECK: call {{.*}} @__dynamic_cast
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z12exact_singleP1A
+B *exact_single(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z9exact_refR1A
+B _ref(A ) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: call {{.*}} @__cxa_bad_cast
+  // CHECK: unreachable
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: ret ptr %[[RESULT]]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z11exact_multiP1A
+H *exact_multi(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[OFFSET_TO_TOP_SLOT:.*]] = getelementptr inbounds i64, ptr %[[VPTR]], i64 -2
+  // CHECK: %[[OFFSET_TO_TOP:.*]] = load i64, ptr %[[OFFSET_TO_TOP_SLOT]]
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 %[[OFFSET_TO_TOP]]
+  // CHECK: %[[DERIVED_VPTR:.*]] = load ptr, ptr %[[RESULT]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[DERIVED_VPTR]], getelementptr inbounds ({ [5 x ptr], [4 x ptr], [4 x ptr], [6 x ptr], [6 x ptr] }, ptr @_ZTV1H, i32 0, inrange i32 0, i32 3)
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return dynamic_cast(a);
+}
Index: clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp

[PATCH] D155857: [clang] fix nonnull warnings during build

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision as: rsmith.
rsmith added a comment.
This revision is now accepted and ready to land.

Seems reasonable to me. Given that this affects all of LLVM I'd like to wait a 
day or so to see if anyone has concerns.




Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:774
+
+  append_if(CMAKE_COMPILER_IS_GNUCXX "-Wno-nonnull" CMAKE_CXX_FLAGS)
+

It'd be useful to leave a comment for future readers that we're disabling this 
due to false positives.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

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

[PATCH] D155857: [clang] fix nonnull warnings during build

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

This branch doesn't look necessary or correct to me. We should never call this 
function on a lazy pointer in offset mode unless we have an external source 
that produced the offset, and it's certainly not appropriate to cast the offset 
to a pointer and return it.

Do you have a testcase that is crashing here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

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

[PATCH] D134475: [clang-cl] Add support for [[msvc::constexpr]] C++11 attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:9591-9594
+  bool IsMSConstexpr = Info.CurrentCall->CanEvalMSConstexpr &&
+   OperatorNew->hasAttr();
   if (OperatorNew->isReservedGlobalPlacementOperator() &&
+  (Info.CurrentCall->isStdFunction() || IsMSConstexpr) && !E->isArray()) {

RIscRIpt wrote:
> rsmith wrote:
> > Do we really need this change? Was our existing check of whether the caller 
> > is in namespace `std` not sufficient for MS' standard library? I'd strongly 
> > prefer not to have a documented, user-visible attribute that gives 
> > permission to use placement new directly.
> Yes, STL's `operator new` is defined in global namespace in [[ 
> https://gist.github.com/RIscRIpt/9f0991f09f97eafc375fc73ea851a81b#file-vcruntime_new-h-L165
>  | vcruntime_new.h ]] (and all includes of this file are made from global 
> namespace).
The existing code is checking whether the caller of `operator new` (eg, 
`std::construct_at`) is in namespace `std`, not whether the `operator new` 
itself is. Does MSVC's `construct_at` need this? It might if it uses a 
placement new indirectly via a function in a different namespace, but it seems 
likely to me that it doesn't.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134475/new/

https://reviews.llvm.org/D134475

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

[PATCH] D134475: [clang-cl] Add support for [[msvc::constexpr]] C++11 attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:9591-9594
+  bool IsMSConstexpr = Info.CurrentCall->CanEvalMSConstexpr &&
+   OperatorNew->hasAttr();
   if (OperatorNew->isReservedGlobalPlacementOperator() &&
+  (Info.CurrentCall->isStdFunction() || IsMSConstexpr) && !E->isArray()) {

Do we really need this change? Was our existing check of whether the caller is 
in namespace `std` not sufficient for MS' standard library? I'd strongly prefer 
not to have a documented, user-visible attribute that gives permission to use 
placement new directly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134475/new/

https://reviews.llvm.org/D134475

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

[PATCH] D134475: [clang-cl] Add support for [[msvc::constexpr]] C++11 attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:5615-5627
+if (canEvalMSConstexpr || isMSConstexpr) {
+  // Diagnose invalid usage of [[msvc::constexpr]] function
+  bool isConstructor = isa(Definition);
+  if (canEvalMSConstexpr) { // !isMSConstexpr
+Info.FFDiag(CallLoc, diag::note_constexpr_invalid_function, 1)
+<< /*IsConstexpr*/ 0 << isConstructor << Definition;
+Info.Note(Definition->getLocation(), diag::note_declared_at);

Given that the intended use case is for usage behind the scenes in the standard 
library, I don't think we should be changing our diagnostic output at all here. 
If the library, as an implementation detail, marks a non-`constexpr` function 
as `[[msvc::constexpr]]`, we shouldn't tell the user to add 
`[[msvc::constexpr]]` to their code to allow it to be called, after all, the 
annotation is an implementation detail of the MS standard library.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134475/new/

https://reviews.llvm.org/D134475

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

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Sema/Lookup.h:228-229
 Other.Paths = nullptr;
-Other.Diagnose = false;
+Other.DiagnoseAccess = false;
+Other.DiagnoseAmbiguous = false;
 return *this;

cor3ntin wrote:
> rsmith wrote:
> > cor3ntin wrote:
> > > Does anything break if you remove these two lines? they don't appear 
> > > useful
> > I think these make sense: if we move a lookup result into this one, then 
> > the other lookup result shouldn't issue diagnostics any more. (Otherwise we 
> > could see the same diagnostics twice.)
> The reason I'm asking is that we don't seem consistent about resetting the 
> state of the moved-from lookup, so i  don;t know if moved from lookup are 
> ever reused. and if they are, should we use std::exchange or something along 
> those lines?
I don't think moved-from `LookupResult`s are ever reused, and I don't think 
it's the intent for this function to leave a moved-from result in a state 
suitable for use in further lookups. (If you assign over a moved-from lookup 
result, that'd presumably work fine, though I doubt we ever do that.) The point 
here, I think, is just to leave the source of the move in a state where the 
destructor doesn't have side-effects any more.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155387/new/

https://reviews.llvm.org/D155387

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

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Sema/Lookup.h:228-229
 Other.Paths = nullptr;
-Other.Diagnose = false;
+Other.DiagnoseAccess = false;
+Other.DiagnoseAmbiguous = false;
 return *this;

cor3ntin wrote:
> Does anything break if you remove these two lines? they don't appear useful
I think these make sense: if we move a lookup result into this one, then the 
other lookup result shouldn't issue diagnostics any more. (Otherwise we could 
see the same diagnostics twice.)



Comment at: clang/lib/Sema/SemaOverload.cpp:14930
   LookupQualifiedName(R, Record->getDecl());
-  R.suppressDiagnostics();
+  R.suppressAccessDiagnostics();
 

shafik wrote:
> I was a bit conservative where I changed this but maybe we should do this 
> everywhere.
Most of the other calls in this file all look wrong. `DiagnoseTwoPhaseLookup` 
and `BuildRecoveryCallExpr` are doing error recovery and I think it's 
appropriate for them to suppress all diagnostics, but the rest are doing 
standard-mandated searches, and so should diagnose lookup ambiguity.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155387/new/

https://reviews.llvm.org/D155387

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D154658#4490468 , @rjmccall wrote:

> Oh, this does matter on platforms using pointer authentication, because each 
> vptr field is signed with a constant discriminator derived from the name of 
> the class that introduced it.

Oh, hm. I guess you have an out-of-tree patch for that? I've switched back to 
passing types into `GetVTablePtr`, and stopped using that from 
`emitExactDynamicCast` in favor of directly loading the vtpr after the 
`dynamic_cast`. You would need to disable the exact `dynamic_cast` 
optimization when pointer authentication is enabled for vptrs, but that should 
be a small patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 539776.
rsmith marked an inline comment as done.
rsmith added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: jplehr, sstefan1.

- Avoid emitting the type_info when detecting whether it would be null.
- Bring back class type in GetVTablePtr and instead don't use it here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

Files:
  clang/lib/AST/ExprCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/dynamic-cast-always-null.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact.cpp

Index: clang/test/CodeGenCXX/dynamic-cast-exact.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dynamic-cast-exact.cpp
@@ -0,0 +1,78 @@
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions -std=c++11 -o - | FileCheck %s --implicit-check-not='call {{.*}} @__dynamic_cast'
+struct Offset { virtual ~Offset(); };
+struct A { virtual ~A(); };
+struct B final : Offset, A { };
+
+struct C { virtual ~C(); int c; };
+struct D : A { int d; };
+struct E : A { int e; };
+struct F : virtual A { int f; };
+struct G : virtual A { int g; };
+struct H final : C, D, E, F, G { int h; };
+
+// CHECK-LABEL: @_Z7inexactP1A
+C *inexact(A *a) {
+  // CHECK: call {{.*}} @__dynamic_cast
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z12exact_singleP1A
+B *exact_single(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z9exact_refR1A
+B _ref(A ) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: call {{.*}} @__cxa_bad_cast
+  // CHECK: unreachable
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: ret ptr %[[RESULT]]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z11exact_multiP1A
+H *exact_multi(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[OFFSET_TO_TOP_SLOT:.*]] = getelementptr inbounds i64, ptr %[[VPTR]], i64 -2
+  // CHECK: %[[OFFSET_TO_TOP:.*]] = load i64, ptr %[[OFFSET_TO_TOP_SLOT]]
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 %[[OFFSET_TO_TOP]]
+  // CHECK: %[[DERIVED_VPTR:.*]] = load ptr, ptr %[[RESULT]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[DERIVED_VPTR]], getelementptr inbounds ({ [5 x ptr], [4 x ptr], [4 x ptr], [6 x ptr], [6 x ptr] }, ptr @_ZTV1H, i32 0, inrange i32 0, i32 3)
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return dynamic_cast(a);
+}
Index: clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -std=c++11 -o - | FileCheck %s --check-prefixes=CHECK,EXACT
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -fvisibility=hidden -emit-llvm -std=c++11 -o - | FileCheck %s --check-prefixes=CHECK,INEXACT
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -fapple-kext -emit-llvm -std=c++11 -o - | 

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 538283.
rsmith added a comment.

- Mark gep as inbounds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

Files:
  clang/lib/AST/ExprCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/dynamic-cast-always-null.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact.cpp

Index: clang/test/CodeGenCXX/dynamic-cast-exact.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dynamic-cast-exact.cpp
@@ -0,0 +1,78 @@
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions -std=c++11 -o - | FileCheck %s --implicit-check-not='call {{.*}} @__dynamic_cast'
+struct Offset { virtual ~Offset(); };
+struct A { virtual ~A(); };
+struct B final : Offset, A { };
+
+struct C { virtual ~C(); int c; };
+struct D : A { int d; };
+struct E : A { int e; };
+struct F : virtual A { int f; };
+struct G : virtual A { int g; };
+struct H final : C, D, E, F, G { int h; };
+
+// CHECK-LABEL: @_Z7inexactP1A
+C *inexact(A *a) {
+  // CHECK: call {{.*}} @__dynamic_cast
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z12exact_singleP1A
+B *exact_single(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z9exact_refR1A
+B _ref(A ) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: call {{.*}} @__cxa_bad_cast
+  // CHECK: unreachable
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: ret ptr %[[RESULT]]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z11exact_multiP1A
+H *exact_multi(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[OFFSET_TO_TOP_SLOT:.*]] = getelementptr inbounds i64, ptr %[[VPTR]], i64 -2
+  // CHECK: %[[OFFSET_TO_TOP:.*]] = load i64, ptr %[[OFFSET_TO_TOP_SLOT]]
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 %[[OFFSET_TO_TOP]]
+  // CHECK: %[[DERIVED_VPTR:.*]] = load ptr, ptr %[[RESULT]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[DERIVED_VPTR]], getelementptr inbounds ({ [5 x ptr], [4 x ptr], [4 x ptr], [6 x ptr], [6 x ptr] }, ptr @_ZTV1H, i32 0, inrange i32 0, i32 3)
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return dynamic_cast(a);
+}
Index: clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -std=c++11 -o - | FileCheck %s --check-prefixes=CHECK,EXACT
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -fvisibility=hidden -emit-llvm -std=c++11 -o - | FileCheck %s --check-prefixes=CHECK,INEXACT
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -fapple-kext -emit-llvm -std=c++11 -o - | FileCheck %s --check-prefixes=CHECK,INEXACT
+
+struct A { virtual ~A(); };
+struct B final : A { };
+
+// CHECK-LABEL: @_Z5exactP1A
+B *exact(A *a) {
+  // INEXACT: call 

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith marked an inline comment as done.
rsmith added a comment.

In D154658#4481225 , @rjmccall wrote:

> In D154658#4479213 , @rsmith wrote:
>
>> I think (hope?) we should be able to apply this to a much larger set of 
>> cases. Would it be correct to do this optimization unless the vtable might 
>> be emitted with vague linkage and non-default visibility (that is, unless 
>> we're in the odd case where people expect non-default visibility classes to 
>> be the same type across DSOs)? Or are there cases where we might be using a 
>> vtable that (eg) doesn't even have the right symbol?
>
> I don't know of any problems other than the total failure of vague linkage 
> across DSO boundaries, so if we just treat that as an implicit exception to 
> the vtable uniqueness guarantee in the ABI, I think you've got the condition 
> exactly right: we could do this for any class where either the v-table 
> doesn't have vague linkage or the class's formal visibility is not `default`. 
>  Our experience at Apple with enforcing type visibility is that it's usually 
> good for one or two bug reports a year, but it's pretty easy to explain to 
> users what they did wrong, and Clang's visibility attributes are pretty 
> simple to use.  (`type_visibility` helps a lot with managing tradeoffs.)

I did some checking through the Clang implementation and found another two 
cases:

- under `-fapple-kext`, vague-linkage vtables get emitted in each translation 
unit that references them
- under `-fno-rtti`, identical vtables for distinct types could get merged 
because we emit vtables as `unnamed_addr` (this doesn't affect `dynamic_cast`, 
because `-fno-rtti` also disables `dynamic_cast` entirely)

The good news seems to be that if you don't use any language extensions (type 
visibility, `-fno-rtti`, `-fapple-kext`), then we do actually provide the 
guarantee that the ABI describes. :)

In D154658#4479170 , @rjmccall wrote:

> If there are multiple subobjects of the source type in the destination type, 
> consider just casting to `void*` first instead of doing multiple comparisons.

This turned out to be a little subtle: the vptr comparison we do after the cast 
requires loading a vptr of an object of entirely-unknown type. This is the 
first time we're doing that in IR generation, and `GetVTablePtr` expected to be 
told which class it's loading the vtable for (presumably, so that it can load 
from the right offset within the class). However, the implementation of 
`GetVTablePtr` didn't use the class for anything; it's always at the start for 
all of our supported ABIs. But this patch would make it harder to support an 
ABI that didn't put the vptr at the start of the most-derived object.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 538281.
rsmith added a comment.

- Address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

Files:
  clang/lib/AST/ExprCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/dynamic-cast-always-null.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact.cpp

Index: clang/test/CodeGenCXX/dynamic-cast-exact.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dynamic-cast-exact.cpp
@@ -0,0 +1,78 @@
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions -std=c++11 -o - | FileCheck %s --implicit-check-not='call {{.*}} @__dynamic_cast'
+struct Offset { virtual ~Offset(); };
+struct A { virtual ~A(); };
+struct B final : Offset, A { };
+
+struct C { virtual ~C(); int c; };
+struct D : A { int d; };
+struct E : A { int e; };
+struct F : virtual A { int f; };
+struct G : virtual A { int g; };
+struct H final : C, D, E, F, G { int h; };
+
+// CHECK-LABEL: @_Z7inexactP1A
+C *inexact(A *a) {
+  // CHECK: call {{.*}} @__dynamic_cast
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z12exact_singleP1A
+B *exact_single(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z9exact_refR1A
+B _ref(A ) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr], [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 1, i32 2)
+  // CHECK: %[[RESULT:.*]] = getelementptr i8, ptr %[[PTR]], i64 -8
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: call {{.*}} @__cxa_bad_cast
+  // CHECK: unreachable
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: ret ptr %[[RESULT]]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z11exact_multiP1A
+H *exact_multi(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_FAILED:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[OFFSET_TO_TOP_SLOT:.*]] = getelementptr inbounds i64, ptr %[[VPTR]], i64 -2
+  // CHECK: %[[OFFSET_TO_TOP:.*]] = load i64, ptr %[[OFFSET_TO_TOP_SLOT]]
+  // CHECK: %[[RESULT:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 %[[OFFSET_TO_TOP]]
+  // CHECK: %[[DERIVED_VPTR:.*]] = load ptr, ptr %[[RESULT]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[DERIVED_VPTR]], getelementptr inbounds ({ [5 x ptr], [4 x ptr], [4 x ptr], [6 x ptr], [6 x ptr] }, ptr @_ZTV1H, i32 0, inrange i32 0, i32 3)
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_END:.*]], label %[[LABEL_FAILED]]
+
+  // CHECK: [[LABEL_FAILED]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[RESULT]], %[[LABEL_NOTNULL]] ], [ null, %[[LABEL_FAILED]] ]
+  return dynamic_cast(a);
+}
Index: clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -std=c++11 -o - | FileCheck %s --check-prefixes=CHECK,EXACT
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -fvisibility=hidden -emit-llvm -std=c++11 -o - | FileCheck %s --check-prefixes=CHECK,INEXACT
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -fapple-kext -emit-llvm -std=c++11 -o - | FileCheck %s --check-prefixes=CHECK,INEXACT
+
+struct A { virtual ~A(); };
+struct B final : A { };
+
+// CHECK-LABEL: @_Z5exactP1A
+B *exact(A *a) {
+  // INEXACT: call {{.*}} 

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D154658#4479170 , @rjmccall wrote:

> I don't think it's an intended guarantee of the Itanium ABI that the v-table 
> will be unique, and v-tables are frequently not unique in the presence of 
> shared libraries.

https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vtable-general explicitly 
guarantees this: "[...] However, the virtual table pointers within all the 
objects (instances) of a particular most-derived class point to the same set of 
virtual tables."

> They should be unique for classes with internal linkage, but of course that's 
> a vastly reduced domain for the optimization.

I think (hope?) we should be able to apply this to a much larger set of cases. 
Would it be correct to do this optimization unless the vtable might be emitted 
with vague linkage and non-default visibility (that is, unless we're in the odd 
case where people expect non-default visibility classes to be the same type 
across DSOs)? Or are there cases where we might be using a vtable that (eg) 
doesn't even have the right symbol?

> If there are multiple subobjects of the source type in the destination type, 
> consider just casting to `void*` first instead of doing multiple comparisons.

Ha, that seems obvious in retrospect :) Will do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added a reviewer: rjmccall.
Herald added subscribers: nlopes, mgrang.
Herald added a project: All.
rsmith requested review of this revision.
Herald added subscribers: cfe-commits, wangpc.
Herald added a project: clang.

- When the destination is a final class type that does not derive from the 
source type, the cast always fails and is now emitted as a null pointer or call 
to __cxa_bad_cast.

- When the destination is a final class type that does derive from the source 
type, emit a direct comparison against the corresponding base class vptr 
value(s). There may be more than one such value in the case of multiple 
inheritance; check them all.

For now, this is supported only for the Itanium ABI. I expect the same thing is
possible for the MS ABI too, but I don't know what guarantees are made about
vfptr uniqueness.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154658

Files:
  clang/lib/AST/ExprCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/dynamic-cast-always-null.cpp
  clang/test/CodeGenCXX/dynamic-cast-exact.cpp

Index: clang/test/CodeGenCXX/dynamic-cast-exact.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dynamic-cast-exact.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -I%S %s -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions -std=c++11 -o - | FileCheck %s --implicit-check-not='call {{.*}} @__dynamic_cast'
+struct A { virtual ~A(); };
+struct B final : A { };
+
+struct C { virtual ~C(); int c; };
+struct D : A { int d; };
+struct E : A { int e; };
+struct F : virtual A { int f; };
+struct G : virtual A { int g; };
+struct H final : C, D, E, F, G { int h; };
+
+// CHECK-LABEL: @_Z7inexactP1A
+C *inexact(A *a) {
+  // CHECK: call {{.*}} @__dynamic_cast
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z12exact_singleP1A
+B *exact_single(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_NULL:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 0, i32 2)
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_SUCCESS:.*]], label %[[LABEL_NULL]]
+
+  // CHECK: [[LABEL_SUCCESS]]:
+  // CHECK: br label %[[LABEL_END:.*]]
+
+  // CHECK: [[LABEL_NULL]]:
+  // CHECK: br label %[[LABEL_END]]
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: phi ptr [ %[[PTR]], %[[LABEL_SUCCESS]] ], [ null, %[[LABEL_NULL]] ]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z9exact_refR1A
+B _ref(A ) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_NULL:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [4 x ptr] }, ptr @_ZTV1B, i32 0, inrange i32 0, i32 2)
+  // CHECK: br i1 %[[MATCH]], label %[[LABEL_SUCCESS:.*]], label %[[LABEL_NULL]]
+
+  // CHECK: [[LABEL_SUCCESS]]:
+  // CHECK: br label %[[LABEL_END:.*]]
+
+  // CHECK: [[LABEL_NULL]]:
+  // CHECK: call {{.*}} @__cxa_bad_cast
+  // CHECK: unreachable
+
+  // CHECK: [[LABEL_END]]:
+  // CHECK: ret ptr %[[PTR]]
+  return dynamic_cast(a);
+}
+
+// CHECK-LABEL: @_Z11exact_multiP1A
+H *exact_multi(A *a) {
+  // CHECK: %[[PTR_NULL:.*]] = icmp eq ptr %[[PTR:.*]], null
+  // CHECK: br i1 %[[PTR_NULL]], label %[[LABEL_NULL:.*]], label %[[LABEL_NOTNULL:.*]]
+
+  // CHECK: [[LABEL_NOTNULL]]:
+  // CHECK: %[[VPTR:.*]] = load ptr, ptr %[[PTR]]
+  // CHECK: %[[MATCH_1:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [5 x ptr], [4 x ptr], [4 x ptr], [6 x ptr], [6 x ptr] }, ptr @_ZTV1H, i32 0, inrange i32 1, i32 2)
+  // CHECK: br i1 %[[MATCH_1]], label %[[LABEL_SUCCESS:.*]], label %[[LABEL_CONT_1:.*]]
+
+  // CHECK: [[LABEL_CONT_1]]:
+  // CHECK: %[[MATCH_2:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [5 x ptr], [4 x ptr], [4 x ptr], [6 x ptr], [6 x ptr] }, ptr @_ZTV1H, i32 0, inrange i32 2, i32 2)
+  // CHECK: br i1 %[[MATCH_2]], label %[[LABEL_SUCCESS:.*]], label %[[LABEL_CONT_2:.*]]
+
+  // CHECK: [[LABEL_CONT_2]]:
+  // CHECK: %[[MATCH_3:.*]] = icmp eq ptr %[[VPTR]], getelementptr inbounds ({ [5 x ptr], [4 x ptr], [4 x ptr], [6 x ptr], [6 x ptr] }, ptr @_ZTV1H, i32 0, inrange i32 3, i32 4)
+  // CHECK: br i1 %[[MATCH_3]], label %[[LABEL_SUCCESS:.*]], label %[[LABEL_NULL]]
+
+  // CHECK: [[LABEL_SUCCESS]]:
+  // CHECK: %[[SUCCESS_OFFSET:.*]] = phi i64 [ -16, %[[LABEL_NOTNULL]] ], [ -32, %[[LABEL_CONT_1]] ], [ -48, %[[LABEL_CONT_2]] ]
+  // CHECK: %[[SUCCESS_VALUE:.*]] = getelementptr inbounds i8, ptr %[[PTR]], i64 %[[SUCCESS_OFFSET]]
+  // CHECK: br label 

[PATCH] D153701: [Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914
+  // [P2718R0] Lifetime extension in range-based for loops.
+  //
+  // 6.7.7 [class.temporary] p5:
+  // There are four contexts in which temporaries are destroyed at a different
+  // point than the end of the full-expression.
+  //
+  // 6.7.7 [class.temporary] p6:

This isn't the right way to model the behavior here -- the presence or absence 
of an `ExprWithCleanups` is just a convenience to tell consumers of the AST 
whether they should expect to see cleanups later or not, and doesn't carry an 
implication of affecting the actual temporary lifetimes and storage durations.

The outcome that we should be aiming to reach is that all 
`MaterializeTemporaryExpr`s created as part of processing the 
for-range-initializer are marked as being lifetime-extended by the for-range 
variable. Probably the simplest way to handle that would be to track the 
current enclosing for-range-initializer variable in the 
`ExpressionEvaluationContextRecord`, and whenever a `MaterializeTemporaryExpr` 
is created, if there is a current enclosing for-range-initializer, mark that 
`MaterializeTemporaryExpr` as being lifetime-extended by it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153701/new/

https://reviews.llvm.org/D153701

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Making the `return ESR_Failed;` unconditional looks to be the correct change 
here. We can't continue evaluation past that point because we don't know what 
would be executed next. Unconditionally returning `ESR_Failed` in that 
situation is what the other similar paths through `EvaluateStmt` do.




Comment at: clang/lib/AST/ExprConstant.cpp:4914
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());

I don't think the changes to this function are appropriate, because:

1) The special-casing of `RecoveryExpr` doesn't seem like it can be correct. 
There's no guarantee that we get a `RecoveryExpr` any time we encounter an 
expression that contains errors; error-dependence can be propagated from other 
places, such as types.
2) For other error-dependent expressions, we also can't necessarily compute a 
value.
3) It's not the responsibility of this function to deal with the situation 
where a value is needed and can't be produced -- the responsibility to handle 
that lies with the caller of this function instead. Eg, look at the handling of 
`ReturnStmt` or `DoStmt`.

So I think we should undo all the changes in this function, and only fix 
`SwitchStmt` to properly handle a value-dependent condition.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153296/new/

https://reviews.llvm.org/D153296

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

[PATCH] D154270: [clang][CodeGen] Fix global variables initialized with an inheriting constructor.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Wow, what an amazing bug :)




Comment at: clang/lib/CodeGen/CGDecl.cpp:2459
+  if (!isa(Arg.getAnyValue()))
+Arg.getAnyValue()->setName(D.getName());
 

Is it feasible to only set the name if the value doesn't already have a name, 
or do we give values bad names often enough that it's better to overwrite an 
existing name here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154270/new/

https://reviews.llvm.org/D154270

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

[PATCH] D153375: [Clang] Fix incorrect use of direct initialization with copy initialization

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added inline comments.
This revision now requires changes to proceed.



Comment at: clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p3.cpp:293
+  e = {E::E1};
+  e = {0}; // expected-error {{cannot initialize a value of type 'E' with an 
rvalue of type 'int'}}
+}

This looks valid to me. The language rules say we treat this as `e = E{0};`, 
which we accept.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153375/new/

https://reviews.llvm.org/D153375

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

[PATCH] D153375: [Clang] Fix incorrect use of direct initialization with copy initialization

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15441-15443
 // C++11 5.17p9:
 //   The meaning of x = {v} [...] is that of x = T(v) [...]. The meaning
 //   of x = {} is x = T().

The code change doesn't match this comment, which says we should use direct 
non-list initialization. Though the comment is slightly wrong / out of date: 
the standard text now says:

"an assignment to a scalar, in which case the initializer list shall have at 
most a single element. The meaning of `x = {v}`, where `T` is the scalar type 
of the expression `x`, is that of `x = T{v}`. The meaning of `x = {}` is `x = 
T{}`."

... which says to use direct list initialization to create a temporary in the 
scalar type case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153375/new/

https://reviews.llvm.org/D153375

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

[PATCH] D153962: [clang] Do not discard cleanups while processing immediate invocation

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

LGTM, thank you for doing this!




Comment at: clang/lib/Sema/SemaExpr.cpp:18201
+// Hence, in correct code any cleanup objects created inside current
+// evaluation context must be outside the immediate invocation.
+E = ExprWithCleanups::Create(getASTContext(), E.get(),

This argument makes sense to me, thanks for documenting why this is correct!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153962/new/

https://reviews.llvm.org/D153962

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

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D153003#4458323 , @ChuanqiXu wrote:

> In D153003#4456595 , @rsmith wrote:
>
>> How to we even get into the ODR hasher here? I thought we only applied it to 
>> function and class definitions (to which the ODR does apply).
>
> I think this comes from we add ODRHash for  RecordDecl.

The using-declarations in the testcase aren't inside RecordDecls.

Oh, I guess we're somehow adding a semi-resolved form of the base class type of 
`D` into the ODR hash, which then includes details of the using-declaration. 
That seems wrong -- we should either (preferably) be including only the 
syntactic form of the base specifier (because local syntax is what the ODR 
covers), or the canonical type (which should be the same for both 
using-declarations).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153003/new/

https://reviews.llvm.org/D153003

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

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I think the behavior change for the testcase here is correct, though I'm not 
sure that the patch is getting that behaviour change in the right way. Per 
[temp.type]/1.4 (http://eel.is/c++draft/temp.type#1.4),

> Two template-ids are the same if [...] their corresponding template 
> template-arguments refer to the same template.

so `B` and `B` are the same type. The stricter "same sequence of 
tokens" rule doesn't apply here, because using-declarations are not definitions.

But that also suggests that ODR hashing should not be visiting these template 
arguments in using-declarations at all, because the ODR does not apply to the 
types specified in a namespace-scope using-declaration. How to we even get into 
the ODR hasher here? I thought we only applied it to function and class 
definitions (to which the ODR does apply).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153003/new/

https://reviews.llvm.org/D153003

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

[PATCH] D153294: [clang] Do not create ExprWithCleanups while checking immediate invocation

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

A constant expression (including the immediate invocations generated for 
`consteval` functions) is a full-expression, so destructors should be run at 
the end of evaluating it, not as part of the enclosing expression. That's 
presumably why the code is calling `MaybeCreateExprWithCleanups` -- to 
associate cleanups for the immediate invocation with that `ConstantExpr` rather 
than with the outer context. I think the problem is that we don't have an 
`ExpressionEvaluationContext` wrapping the immediate invocation, so we don't 
save and restore the enclosing cleanup state -- and we can't really wrap it, 
because we don't find out that we're building an immediate invocation until 
after we've already done so. Probably the best way to handle that is to create 
the inner `ExprWithCleanups` for the constant expression, but leave the 
cleanups flags alone so that we also create an outer `ExprWithCleanups`. 
That'll mean we sometimes create an `ExprWithCleanups` that doesn't actually 
run any cleanups, but that's OK, just redundant. One thing we will need to be 
careful about is assigning any block / compound literal cleanups to the right 
`ExprWithCleanups` node. We can figure out where to put them by doing a 
traversal of the `ConstantExpr`'s subexpression to see if it contains the 
`BlockDecl` / `CompoundLiteralExpr` being referenced by the cleanup.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153294/new/

https://reviews.llvm.org/D153294

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

[PATCH] D142388: [clang] Add builtin_nondeterministic_value

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D142388#4414375 , @aaron.ballman 
wrote:

> In D142388#4412521 , @rsmith wrote:
>
>> I have a concern with the name of this builtin. People are going to assume 
>> it produces a nondeterministic value, and use it for seeding random number 
>> generators and similar, and will be surprised when the value produced is 
>> actually deterministic, and, worse, might leak information on the stack that 
>> was left behind by some previous function call. Can we give this a name that 
>> doesn't suggest that it produces its value nondeterministically?
>
> That's a fair point; do you have a suggestion for a name? (I had suggested 
> `__builtin_unspecified_value()` but perhaps that is too focused on the term 
> of art.)

If this means exactly what C++ means by "unspecified value" I'm OK with that, 
but I don't think it does with the current design; `__builtin_???(bool)` can 
seemingly return `std::bit_cast('x')`, which is not an unspecified value 
of type `bool` in C++-land (because it's not a value of type `bool` at all). On 
that subject: can we restrict this to types where every object representation 
is valid, and rule out `bool` and (at least some) enumerations, as well as any 
`_BitInt(N)`s with no unused bits? Either that, or mask off the "bad" bits so 
that it actually produces valid values? Allowing type arguments for which any 
use of the builtin is just immediately UB doesn't seem like a good idea.

I don't like `__builtin_undefined_value` because very commonly "undefined" is 
used to mean something else, in ways that show up in close proximity to this 
builtin.

`__builtin_arbitrary_value` works a bit better for me, but is still quite wrong 
/ misleading, because the value is *not* arbitrary. With either that or 
`__builtin_nondeterminstic_value`, I worry that an old and famous crypto bug 
will be reintroduced:

  int my_random_number = __builtin_nondeterministic_value();
  my_random_number ^= rand();
  my_random_number ^= other_junk;

... where I would expect and //hope// that LLVM will optimize the resulting 
value of `my_random_number` to 0 by picking the exact right "nondeterminstic" 
value every time :)

I think `__builtin_any_value` works pretty well, and emphasizes that this can 
really return any value. I'd also be OK with `__builtin_convenient_value`, to 
emphasize that the compiler will pick something that's convenient for it, but I 
prefer `__builtin_any_value`.




Comment at: clang/docs/LanguageExtensions.rst:3087
+
+``__builtin_nondeterministic_value`` returns a valid nondeterministic value of 
the same type as the provided argument.
+

xbolva00 wrote:
> This text really sells this new builtin as a random value generator..
How about something more like this:

> ``__builtin_any_value`` returns an arbitrary value of the same type as the 
> provided argument.
>
> [...]
>
> **Description**:
>
> Each call to ``__builtin_any_value`` returns an arbitrary value of the type 
> given by the argument. The returned value will be chosen in a way that is 
> convenient for the compiler, and may for example be a value left behind in a 
> register or on the stack from a previous operation, or a value that causes a 
> later calculation to always produce the same result. This function will not 
> necessarily return a meaningful value if there are bit-patterns for the given 
> type that do not correspond to meaningful values.

(I really want this to also not say it will produce a "valid value", because 
the valid values for a type are not in general known to the frontend. For a 
pointer, for example, this can return literally any bit pattern, and most of 
those will not be valid in any meaningful sense.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142388/new/

https://reviews.llvm.org/D142388

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

[PATCH] D142388: [clang] Add builtin_nondeterministic_value

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I have a concern with the name of this builtin. People are going to assume it 
produces a nondeterministic value, and use it for seeding random number 
generators and similar, and will be surprised when the value produced is 
actually deterministic, and, worse, might leak information on the stack that 
was left behind by some previous function call. Can we give this a name that 
doesn't suggest that it produces its value nondeterministically?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142388/new/

https://reviews.llvm.org/D142388

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

[PATCH] D148700: [clang] Add support for “regular” keyword attributes

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D148700#4401353 , @jyknight wrote:

> Yes, standard attributes aren't supposed to be used for things which affect 
> the type system (although, we certainly have many, already, which do, since 
> we expose most GCC-syntax attributes also as C++-standard attribute syntax!)

The rule that standard attributes don't affect semantics only applies to 
attributes specified by the language standard. There is no expectation that 
vendor attributes avoid such effects. In particular, I'm concerned by this in 
the description of this change:

In D148700 , @rsandifo-arm wrote:

> Really, the “only” thing wrong with using standard attributes is that 
> standard attributes cannot affect semantics.

If the only reason for this patch series is an idea that vendor attributes 
using `[[...]]` syntax can't affect program semantics, then I think this change 
is not justified, because vendor attributes using `[[...]]` syntax can and 
usually do affect program semantics. But the documentation change here makes 
the point that using a keyword attribute may be as good idea in cases where you 
would always want compilation to fail on a compiler that doesn't understand the 
annotation, rather than the annotation being ignored (likely with a warning), 
so maybe that's reasonable justification for this direction.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148700/new/

https://reviews.llvm.org/D148700

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

[PATCH] D151952: [clang] adds `__type_pack_index` so we can get a type's parameter pack index

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Taking a step back for a moment: what is the intended use case for this? My 
concern is that most of the time you're going to want to make O(N) such queries 
into a pack of N elements, resulting in O(N^2) compile time, much like we 
regrettably get from uses of `__type_pack_element`. If we can avoid the regret 
in this case by providing a different intrinsic, perhaps we should do so. (For 
example, we could take two packs of types, and produce a sequence of integers 
giving the indexes of each of the first types in the second list, in O(N) time. 
And similarly we could add a `__type_pack_elements` that takes a pack of types 
and a pack of indexes and performs N indexing operations at once, in O(N) time, 
rather than having the program do it in O(N^2) time.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151952/new/

https://reviews.llvm.org/D151952

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

[PATCH] D134334: [Clang] Fix crash in isCXXDeclarationSpecifier when attempting to annotate template name

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Parse/ParseTentative.cpp:1553-1554
 return TPResult::Error;
-  if (Tok.isNot(tok::identifier))
+  if (NextToken().isNot(tok::identifier))
 break;
 }

shafik wrote:
> cor3ntin wrote:
> > rsmith wrote:
> > > This doesn't seem correct to me. If we had `scope::foo bar`, and we 
> > > annotate `scope::foo` as a type, then this will get confused by the next 
> > > token now being an (unrelated) identifier. This code is trying to detect 
> > > if an annotation was performed, so I think it intended to check if the 
> > > current token's kind has changed, like is done on line 1295.
> > The confusing bit is that Tok is always an annotated scope already here 
> > (L1598), so TryAnnotateName should not modify that first token (unless 
> > TryAnnotateTypeOrScopeTokenAfterScopeSpec can somehow replace the current 
> > annot_cxxscope by another one, which i don't think can happen?) 
> Ok using `tok::annot_cxxscope` also works and I agree it makes sense as well, 
> `check-clang` also passes.
> 
> So then is the assert below wrong?
> 
> ```
>   // Annotated it, check again.
>   assert(Tok.isNot(tok::annot_cxxscope) ||
>  NextToken().isNot(tok::identifier));
> ```
> 
> It looks like it will work by accident for most cases b/c it checks 
> `tok::annot_cxxscope` first. 
> The confusing bit is that Tok is always an annotated scope already here 
> (L1598), so TryAnnotateName should not modify that first token (unless 
> TryAnnotateTypeOrScopeTokenAfterScopeSpec can somehow replace the current 
> annot_cxxscope by another one, which i don't think can happen?)

Yeah, I think `TryAnnotateTypeOrScopeToken` shouldn't ever replace an 
`annot_cxxscope` token with a different `annot_cxxscope` token representing a 
longer scope specifier -- an `annot_cxxscope` token should always be as long as 
it can be. But it might replace the `annot_cxxscope` token with an 
`annot_typename`, in which case we want to jump out to line 1671 and try again.

> So then is the assert below wrong?

I think it's right -- we either reach the assert if we replace the 
`annot_cxxscope` with something else (an `annot_typename`), in the 
`ANK_TemplateName` case, or if we've successfully annotated the name (as one of 
various non-identifier things), in the `ANK_Success` case. In either case, we 
only reach the assert if we successfully replaced the identifier with an 
annotation token, so the assert should succeed.

And the point of the assert, I think, is to ensure that the recursive call to 
`isCXXDeclarationSpecifier` cannot reach this same codepath again and recurse 
forever, so checking the same condition that we checked on entry seems 
appropriate.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134334/new/

https://reviews.llvm.org/D134334

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

[PATCH] D134334: [Clang] Fix crash in isCXXDeclarationSpecifier when attempting to annotate template name

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Parse/ParseTentative.cpp:1553-1554
 return TPResult::Error;
-  if (Tok.isNot(tok::identifier))
+  if (NextToken().isNot(tok::identifier))
 break;
 }

This doesn't seem correct to me. If we had `scope::foo bar`, and we annotate 
`scope::foo` as a type, then this will get confused by the next token now being 
an (unrelated) identifier. This code is trying to detect if an annotation was 
performed, so I think it intended to check if the current token's kind has 
changed, like is done on line 1295.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134334/new/

https://reviews.llvm.org/D134334

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

[PATCH] D151553: [clang] Fix consteval operators in template contexts

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/TreeTransform.h:11940-11943
-  ExprResult Callee = getDerived().TransformExpr(E->getCallee());
-  if (Callee.isInvalid())
-return ExprError();
-

Fznamznon wrote:
> cor3ntin wrote:
> > I don't understand why we would not need to transform the callee. do we do 
> > that elsewhere?
> For example, the code above for call and subscript operators never transforms 
> the callee.
> This `TransFormExpr` call  seems to be a no-op for overloaded operator call 
> case, and the result never ends up in the resulting ast.
> 
To me it looks like we need to transform the callee in order to transform the 
declarations in the `UnresolvedLookupExpr`, if the operator has such functions. 
For example, in:

```
namespace A {
  template T f(T t) {
T operator+(T, T);
return t + t;
  }
}
namespace B {
  struct X {};
}
void g(B::X x) { A::f(x); }
```

... we need to transform the `UnresolvedLookupExpr` for the `t + t` to map from 
the `operator+` declaration in the `A::f` template to the instantiated 
`operator+` declaration in `A::f`.

But I think this patch is going in the right direction. We don't *actually* 
want to transform the callee; all we want to do is to transform the function or 
list of functions contained in the callee to form an `UnresolvedSet` of 
candidates found during the template parse. Building and then throwing away a 
`DeclRefExpr` or `UnresolvedLookupExpr` denoting that function or set of 
functions is both a waste of time and (as demonstrated by the bug being 
addressed here) problematic.

Instead of calling `TransformExpr` on the callee, we should be calling 
`TransformOverloadExprDecls` if the callee is an `UnresolvedLookupExpr` or 
`TransformDecl` if it's a `DeclRefExpr`, and I think 
`RebuildCXXOperatorCallExpr` should be changed to take an `UnresolvedSet` and a 
`RequiresADL` flag, and it looks like we can then remove the `OrigCallee` 
parameter, if we pass in some extra source locations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151553/new/

https://reviews.llvm.org/D151553

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

[PATCH] D150212: [clang][Sema] Improve diagnostics for auto return type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.

LGTM too.




Comment at: clang/test/SemaCXX/auto-type-from-cxx.cpp:31-43
+class Abstract{
+  public:
+void fun();
+virtual void vfun()=0;
+void call(){getCaller()(*this);} // expected-note {{in 
instantiation of function template specialization 
'TestDeductionFail::getCaller' requested here}}
+};
+

The end of this test seems unnecessary. Is there something else you want this 
test to cover, or can it be simplified?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150212/new/

https://reviews.llvm.org/D150212

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

[PATCH] D150212: [clang][Sema] Improve diagnostics for auto return type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Sema/TemplateDeduction.h:338
 public:
-  TemplateSpecCandidateSet(SourceLocation Loc, bool ForTakingAddress = false)
   : Loc(Loc), ForTakingAddress(ForTakingAddress) {}

Do we need to allow the location to be set after the set is created? Giving 
this type an additional state where the location is absent seems error prone, 
and in the uses below it looks easy to set the location in the caller instead 
of the callee.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150212/new/

https://reviews.llvm.org/D150212

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Sorry for the late comments.




Comment at: clang/lib/Sema/SemaInit.cpp:167-177
 } else if (ParenExpr *PE = dyn_cast(E)) {
   E = PE->getSubExpr();
 } else if (UnaryOperator *UO = dyn_cast(E)) {
   assert(UO->getOpcode() == UO_Extension);
   E = UO->getSubExpr();
 } else if (GenericSelectionExpr *GSE = dyn_cast(E)) {
   E = GSE->getResultExpr();

The duplication between this and `IgnoreParensSingleStep` is a code smell; can 
we expose `IgnoreParensSingleStep` and call it from here?



Comment at: clang/lib/Sema/SemaInit.cpp:8511
+Expr *Init = Args[0];
+S.Diag(Init->getBeginLoc(), diag::ext_init_from_predefined) << Init;
+  }

We won't reach this case for `const char arr[] = {__func__};` because in that 
case `Args[0]` will be an `InitListExpr` instead. Right now we accept this with 
no warning in MS extensions mode:

```
void f() {
  const char c[] = {__func__};
}
```

We should perform this check in the handling of `SK_StringInit` instead, in 
order to properly address that case.

Also, this will only catch the case where the argument is an unparenthesized 
predefined expression. You'll need to do something like repeatedly calling 
`IgnoreParensSingleStep` looking for a `PredefinedExpr` to cope with things 
like:

```
void g() {
  const char c[] = (__func__);
  const char d[] = _Generic(0, int: __func__);
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146764/new/

https://reviews.llvm.org/D146764

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

[PATCH] D150875: Make dereferencing a void* a hard-error instead of warn-as-error

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D150875#4353384 , @erichkeane 
wrote:

> We are the only of the major compilers with this "extension" (I hesitate to 
> call it that, as I'm not sure this FITS in the 'extension's permitted by 
> standard)

I'm not objecting to removing this extension, but... do you have reason to 
doubt that it's conforming, or just a lack of confidence that it is? (If the 
`SFINAEFailure` change wasn't enough, then it's not clear to me why this change 
would be. We use a `SFINAEFailure` diagnostic for other extensions, and if 
that's not sufficient for conformance then we probably have a lot of 
conformance gaps of this kind.)




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6985
+def err_typecheck_indirection_through_void_pointer_cpp
+: Error<"ISO C++ does not allow indirection on operand of type %0">;
 def warn_indirection_through_null : Warning<

We normally only use this "ISO C++ does not allow" phrasing for extensions 
(with the implication being that ISO C++ doesn't allow it, but Clang does). Can 
you rephrase the diagnostic too, to remove those unnecessary words?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150875/new/

https://reviews.llvm.org/D150875

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

[PATCH] D149713: [Sema] Avoid emitting warnings for constant destruction.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM with the extra test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149713/new/

https://reviews.llvm.org/D149713

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

[PATCH] D149713: [Sema] Avoid emitting warnings for constant destruction.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I think this would be an interesting test:

  struct S {
/*not constexpr*/ S();
constexpr ~S() {}
  };
  S s; // no warning
  
  struct T {
/*not constexpr*/ T();
constexpr ~T() { if (b) {} }
bool b;
  };
  T t; // expected-warning {{exit-time destructor}}

(For a non-`constexpr` variable with a `constexpr` destructor, our behaviour 
depends on whether the destructor call is a constant expression.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149713/new/

https://reviews.llvm.org/D149713

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

[PATCH] D149514: Check if First argument in _builtin_assume_aligned_ is of pointer type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

There's some corresponding code in `lib/CodeGen` that can be deleted too:

 case Builtin::BI__builtin_assume_aligned: {
   const Expr *Ptr = E->getArg(0);
   Value *PtrValue = EmitScalarExpr(Ptr);
  - if (PtrValue->getType() != VoidPtrTy)
  -   PtrValue = EmitCastToVoidPtr(PtrValue);
   Value *OffsetValue =
 (E->getNumArgs() > 2) ? EmitScalarExpr(E->getArg(2)) : nullptr;




Comment at: clang/lib/Sema/SemaChecking.cpp:7979-7986
   {
 ExprResult FirstArgResult =
 DefaultFunctionArrayLvalueConversion(FirstArg);
-if (FirstArgResult.isInvalid())
+if (checkBuiltinArgument(*this, TheCall, 0))
   return true;
+/// In-place updation of FirstArg by checkBuiltinArgument is ignored.
 TheCall->setArg(0, FirstArgResult.get());

This still seems to be more complex than necessary. If we can't just do this, 
we should have a comment explaining why. (Eg, does CodeGen really want the 
final conversion from `T*` to `const void*` to not be in the AST?)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149514/new/

https://reviews.llvm.org/D149514

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

[PATCH] D148997: [clang] Add a new annotation token: annot_repl_input_end

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This looks like a nice improvement to me.




Comment at: clang/lib/Interpreter/IncrementalParser.cpp:162
+  if (P->getCurToken().is(tok::annot_repl_input_end)) {
+P->ConsumeAnyToken();
 // FIXME: Clang does not call ExitScope on finalizing the regular TU, we





Comment at: clang/lib/Interpreter/IncrementalParser.cpp:254-268
   if (PP.getLangOpts().DelayedTemplateParsing) {
 // Microsoft-specific:
 // Late parsed templates can leave unswallowed "macro"-like tokens.
 // They will seriously confuse the Parser when entering the next
 // source file. So lex until we are EOF.
 Token Tok;
 do {

Not really related to this patch, but in `DelayedTemplateParsing` mode this 
code appears to expect two `annot_repl_input_end` tokens in a row.



Comment at: clang/lib/Parse/ParseStmt.cpp:546-556
+  Token *CurTok = nullptr;
+  // If we're parsing an ExprStmt and the last semicolon is missing and the
+  // incremental externsion is enabled and we're reaching the end, consider we
+  // want to do value printing. Note this is only enable in C++ mode
+  // since part of the implementation requires C++ language features.
+  //
+  // Note we shouldn't eat the token since the callback need it.

I don't think we need to check whether incremental processing is enabled here 
-- if not, we shouldn't see the "end of REPL input" token at all.



Comment at: clang/lib/Parse/Parser.cpp:620-621
   // processing
-  if (PP.isIncrementalProcessingEnabled() && Tok.is(tok::eof))
-ConsumeToken();
+  if (PP.isIncrementalProcessingEnabled() && Tok.is(tok::annot_repl_input_end))
+ConsumeAnnotationToken();
 

Do we need to do this here? `IncrementalParser` already seems to take care of 
this, and the logic here would be easier to reason about if `Parser` never 
steps past an `annot_repl_input_end` token, and such tokens instead are only 
ever consumed by the REPL.

Are there other users of incremental processing mode, other than the REPL / 
`IncrementalParser`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148997/new/

https://reviews.llvm.org/D148997

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

[PATCH] D149516: [Sema] `setInvalidDecl` for error deduction declaration

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I wonder if there are any cases where the "deduction guide" is so broken that 
we shouldn't be creating a `CXXDeductionGuideDecl` at all. For example, if 
there's no trailing return type, then the declaration is syntactically invalid 
(and looks more like a constructor than a deduction guide), and it's not clear 
to me whether creating a `CXXDeductionGuideDecl` really makes sense.

In D149516#4308203 , @shafik wrote:

> I have some questions, why don't we always call 
> `CheckDeductionGuideDeclarator` when calling `CXXDeductionGuideDecl::Create`, 
> I felt like perhaps `CheckDeductionGuideDeclarator` should be rolled into 
> `CXXDeductionGuideDecl::Create` but then I noticed they are not paired up in 
> other locations.

`CXXDeductionGuideDecl::Create` is the low-level AST primitive for creating a 
`CXXDeductionGuideDecl`. The responsibility for validating the declaration and 
issuing diagnostics doesn't belong at that level, it belongs in `Sema`. The 
other callers are:

- When `Sema` creates an implicit deduction guide from a constructor. In this 
case `Sema` knows it's valid by construction.
- When `Sema` instantiates a deduction guide. In this case `Sema` knows it's 
valid because the template was.




Comment at: clang/lib/Sema/SemaDecl.cpp:9202
   << TrailingRequiresClause->getSourceRange();
-SemaRef.CheckDeductionGuideDeclarator(D, R, SC);
+bool IsValid = SemaRef.CheckDeductionGuideDeclarator(D, R, SC);
 

This is reversed from the normal Clang convention (where `true` indicates that 
an error was produced).



Comment at: clang/lib/Sema/SemaDecl.cpp:9204
 
-return CXXDeductionGuideDecl::Create(SemaRef.Context, DC, D.getBeginLoc(),
+CXXDeductionGuideDecl* DGD = 
CXXDeductionGuideDecl::Create(SemaRef.Context, DC, D.getBeginLoc(),
  ExplicitSpecifier, NameInfo, R, TInfo,

Use `auto` here.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11141-11145
 if (!Chunk.Fun.hasTrailingReturnType()) {
+  IsValid = false;
   Diag(D.getName().getBeginLoc(),
diag::err_deduction_guide_no_trailing_return_type);
   break;

Can we just return immediately here? I don't see a reason to keep going -- this 
error may well indicate that the developer didn't actually mean to declare a 
deduction guide in the first place, so the diagnostic on a function body below 
is likely to not be useful.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11196
   if (D.isFunctionDefinition())
 Diag(D.getIdentifierLoc(), diag::err_deduction_guide_defines_function);
+  return IsValid;

Should we return that we had an error after this one too?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149516/new/

https://reviews.llvm.org/D149516

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

[PATCH] D148274: [clang] Fix overly aggressive lifetime checks for parenthesized aggregate initialization

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

LGTM, I'll leave approval to @shafik :)




Comment at: clang/lib/Sema/SemaInit.cpp:5379
+} else {
   ArrayLength = Args.size();
+}

Can we assert that we have an `IncompleteArrayType` here? (That is, that we 
don't have a `DependentSizedArrayType`.)



Comment at: clang/lib/Sema/SemaInit.cpp:5513
+  InitializedEntity SubEntity =
+  InitializedEntity::InitializeMember(FD, );
+  InitializationKind SubKind = InitializationKind::CreateValue(

Is there a reason to not use `InitializeMemberFromParenAggInit` here? That'd 
mean we use the same `InitializedEntity` for every case in this loop, and could 
pull it out of the individual `if` branches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148274/new/

https://reviews.llvm.org/D148274

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

[PATCH] D149264: GH62362: Handle constraints on "Using" inherited constructors

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:138-144
+bool SkipForSpecialization,
+bool ForConstraintInstantiation) {
   if (!ClassTemplSpec->isClassScopeExplicitSpecialization()) {
 // We're done when we hit an explicit specialization.
 if (ClassTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization 
&&
-!isa(ClassTemplSpec))
+!isa(ClassTemplSpec) &&
+!ForConstraintInstantiation)

rsmith wrote:
> It seems to me that a namespace-scope explicit specialization shouldn't pick 
> up template arguments, even during constraint checking:
> 
> ```
> template struct A {};
> template<> struct A {
>   template void f() requires U::value;
> };
> ```
> Constraint checking for `A::f` should produce a single level of 
> template arguments, ``, not two layers `, `, because `U` in 
> `U::value` is a depth-0 index-0 parameter.
Complete testcase:
```
struct X {}; struct Y { static constexpr bool value = true; };
template struct A {};
template<> struct A {  
  template void f() requires U::value;
};   
void g(A a) { a.f(); }
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149264/new/

https://reviews.llvm.org/D149264

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

[PATCH] D149264: GH62362: Handle constraints on "Using" inherited constructors

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I think that the bugs that this was aiming to fix were fixed by 
rG1e43349e321694d7fee3d77cb691887ad67fb5d7 
, which 
means that we should not ask whether the constraints of an inherited 
constructor are satisfied any more. Instead, overload resolution will look at 
whether the constraints of the original constructors are satisfied. (More 
broadly, I wonder whether `DiagnoseUseOfDecl` should be considering constraints 
at all -- the model in the standard is that any time you name a function, you 
go through overload resolution, which considers constraints, so 
`DiagnoseUseOfDecl` should only ever be duplicating work that's already been 
done by overload resolution, but it's possible we're not entirely following 
that model. Still, we should be able to avoid redoing the constraint 
satisfaction checks in the common case where overload resolution already did 
them.)

That said, I think this change would still be correct if for whatever reason we 
started reaching this codepath for inheriting constructors again.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:138-144
+bool SkipForSpecialization,
+bool ForConstraintInstantiation) {
   if (!ClassTemplSpec->isClassScopeExplicitSpecialization()) {
 // We're done when we hit an explicit specialization.
 if (ClassTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization 
&&
-!isa(ClassTemplSpec))
+!isa(ClassTemplSpec) &&
+!ForConstraintInstantiation)

It seems to me that a namespace-scope explicit specialization shouldn't pick up 
template arguments, even during constraint checking:

```
template struct A {};
template<> struct A {
  template void f() requires U::value;
};
```
Constraint checking for `A::f` should produce a single level of template 
arguments, ``, not two layers `, `, because `U` in `U::value` is a 
depth-0 index-0 parameter.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:217-219
+  if (Ctor->isImplicit() && Ctor->isInheritingConstructor() &&
+  Ctor->getInheritedConstructor().getConstructor())
+Ctor = Ctor->getInheritedConstructor().getConstructor();

These checks can be simplified.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149264/new/

https://reviews.llvm.org/D149264

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

[PATCH] D148372: [clang] add diagnose when member function contains invalid default argument

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Are there any calls to `Sema::ActOnParamDefaultArgumentError` left? If not, can 
you also remove the function?

Otherwise this LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148372/new/

https://reviews.llvm.org/D148372

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

[PATCH] D148274: [clang] Fix overly aggressive lifetime checks for parenthesized aggregate initialization

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:5364-5368
 if (const ConstantArrayType *CAT =
 S.getASTContext().getAsConstantArrayType(Entity.getType()))
   ArrayLength = CAT->getSize().getZExtValue();
 else
   ArrayLength = Args.size();

What happens if the array is of `VariableArrayType` or 
`DependentSizedArrayType`? I guess we shouldn't get here in the latter case, 
but the former case seems possible, and presumably shouldn't result in 
constructing a value of `ConstantArrayType`. 
[Test](https://godbolt.org/z/377TWzn7r):

```
constexpr int f(int n, int i) {
int arr[n](1, 2, 3);
return arr[i];
}

constexpr int a = f(1, 2);
constexpr int b = f(4, 3);
```

GCC appears to leave the type alone in this case, and treats the evaluation as 
UB if `n` is less than the number of initializers given. That matches what GCC 
does for a `{...}` initializer of a VLA. We should probably match what Clang 
does for `{...}` initialization of a VLA and reject.



Comment at: clang/lib/Sema/SemaInit.cpp:5391-5393
+ResultType = S.Context.getConstantArrayType(
+AT->getElementType(), llvm::APInt(/*numBits=*/32, ArrayLength),
+/*SizeExpr=*/nullptr, ArrayType::Normal, 0);

It would be nice to use the original type here in the case where we didn't add 
an array bound, so we preserve type sugar (typedefs etc). Also, do we ever need 
to preserve type qualifiers from the original entity's type?



Comment at: clang/lib/Sema/SemaInit.cpp:5401
+InitializedEntity SubEntity =
+InitializedEntity::InitializeBase(S.getASTContext(), , false);
+if (EntityIndexToProcess < Args.size()) {

Does this have the same wrong-lifetime-kind problem as members did?



Comment at: clang/lib/Sema/SemaInit.cpp:5476
+  InitializedEntity SubEntity =
+  InitializedEntity::InitializeMemberFromDefaultMemberInitializer(
+  FD);

Does this entity kind do the right thing for lifetime warnings? (I'm not sure 
why this is a distinct kind of `InitializedEntity`; the thing that changes here 
is not the entity, it's how it's initialized.)



Comment at: clang/lib/Sema/SemaInit.cpp:5486-5487
+  //   The remaining elements...otherwise are value initialzed
+  InitializedEntity SubEntity =
+  InitializedEntity::InitializeMember(FD, );
+  InitializationKind SubKind = InitializationKind::CreateValue(

Is there any possibility of lifetime warnings here? I don't *think* value 
initialization can ever create problems, but it would seem more obviously right 
to use the parenthesized aggregate initialization entity kind here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148274/new/

https://reviews.llvm.org/D148274

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

[PATCH] D148372: [clang] add diagnose when member function contains invalid default argument

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:7385
   if (!ConsumeAndStoreInitializer(*DefArgToks, CIK_DefaultArgument)) {
 DefArgToks.reset();
+Diag(ArgStartLoc, diag::err_expected) << "initializer";

I think we should just remove this `reset` call. That way, the tokens we've 
accumulated will get parsed when we come to later process the default argument, 
and we can produce the proper error at that time, such as diagnosing a missing 
`:`. That's what we do for default member initializers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148372/new/

https://reviews.llvm.org/D148372

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

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:746
+ diag::note_template_class_instantiation_here)
+<< CTD << Active->InstantiationRange;
   } else {

This diagnostic won't include the template arguments that we're using to 
instantiate, which seems like important information.

Do you know where we're creating the `InstantiatingTemplate` instance 
corresponding to this diagnostic? Usually we pass in the declaration whose 
definition we're instantiating for a `TemplateInstantiation` record; I wonder 
if we could do the same for this case.



Comment at: clang/test/SemaCXX/cxx1z-copy-omission.cpp:175
+
+// Make sure we don't consider conversion functions for guaranteed copy elision
+namespace GH39319 {

We should also test that the right function is actually being selected and 
called. For example:

```
struct A {
  A();
  A(const A&) = delete;
};
struct B {
  operator A();
} C;
A::A() : A(C) {}
```
... should be rejected because it calls the deleted copy constructor. (Or you 
could test this via constant evaluation or look at the generated code, but this 
is probably the simplest way.)



Comment at: clang/test/SemaCXX/cxx1z-copy-omission.cpp:193
+template
+class B3 : A3 // expected-error {{expected '{' after base class list}}
+  template()> // expected-warning 2{{use of function template 
name with no prior declaration in function call with explicit}}

Do you need to omit the `{` here as part of this test? This will cause problems 
for people editing the file due to mismatched braces, and makes this test 
fragile if our error recovery changes. It's best for the test case to be free 
of errors other than the one(s) being exercised. (Same for missing `;` errors 
below.)

If these errors are necessary to trigger the bug you found, I wonder if there's 
a problem with our error recovery. (Maybe we create a "bad" 
`InstantiatingTemplate` record during error recovery, for example.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148474/new/

https://reviews.llvm.org/D148474

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

[PATCH] D147782: [clang] Fix 'operator=' lookup in nested class

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Ah, I see.

In D147782#4251185 , @J-Camilleri 
wrote:

> 2. Have the `IdResolver` declarations ordered by scope, going from most 
> nested to least nested.

This is the intended behavior. But `Sema::PushOnScopeChains` doesn't correctly 
handle declarations being added in non-lexical order, except for label 
declarations, which are special-cased.

I think probably the cleanest fix would be to add a flag to `PushOnScopeChains` 
to indicate that the scope to which we're adding the name may not be the 
innermost scope, and so we should find the correct insertion point (like we 
already do for `LabelDecl`s). We should then set that flag when calling 
`PushOnScopeChains` with a scope that's not necessarily the innermost scope, 
such as from `Sema::DeclareImplicit*`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147782/new/

https://reviews.llvm.org/D147782

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

[PATCH] D147782: [clang] Fix 'operator=' lookup in nested class

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

What *should* happen here is that the lookup for `operator=` in 
`Inner::invokeAssign` should walk up the enclosing `Scope`s and `DeclContext`s, 
traversing both the results from the `IdResolver` and the results from 
`DeclContext` lookups. First, we should look in the scope corresponding to 
`invokeAssign`, where we find no results in the `IdResolver` (and there's no 
corresponding `DeclContext` so there's nowhere else to look). Then we should 
look in the scope corresponding to `Inner`, where again there's nothing in the 
`IdResolver` at that scope, but lookup in the `DeclContext` should find the 
member `operator=`. And so it shouldn't matter that the `Outer` scope has a 
lookup result in the `IdContext`, because that scope is never the one that 
we're doing a lookup within.

It sounds like the implicit lazy declaration of the `operator=` might be adding 
the name to the wrong scope, or perhaps the set of nested `Scope`s at the point 
where we parse `Inner::invokeAssign` is misconfigured. In 
`Sema::DeclareImplicitCopyAssignment`, we call `getScopeForContext(ClassDecl)` 
to find the right scope, and inject the name into that scope. I suspect that we 
could simply delete that entirely, given that any lookup for a class member 
name will also look into the `DeclContext` and doesn't need to find results in 
the `Scope` (in the `IdResolver`), but it still seems valuable to figure out 
why this isn't working. `getScopeForContext` finds the *innermost* scope 
corresponding to the given class, so if we had multiple enclosing scopes for 
the same class, we might get into some trouble, and it seems possible that that 
happens when reentering scopes when doing delayed parsing of class members. The 
scopes for delayed parsing are created by `Parser::ReenterClassScopeRAII`, and 
it looks like it's doing the right thing, but that's where I'd start looking.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147782/new/

https://reviews.llvm.org/D147782

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

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D147655#4251042 , @aaron.ballman 
wrote:

> In D147655#4250922 , @royjacobson 
> wrote:
>
>> In D147655#4250056 , @rsmith wrote:
>>
>>> There has not been any stable ABI from any compiler targeting the Itanium 
>>> C++ ABI for constrained templates prior to this change. I don't think we 
>>> need to worry too much about people using unfinished compiler features 
>>> being broken when those features are finished.
>>
>> The ABI has been stable for quite some time and people have been using 
>> concepts with it for almost 3 years now. I'm not sure we can still break ABI 
>> this easily. But then, I also have no data to say we can't.
>
> We've never claimed full support for concepts, so those folks would be 
> relying on an unstable ABI. However, if it turns out this causes significant 
> pain in practice, perhaps we could use ABI tags to give folks the older ABI? 
> I'd prefer to avoid that in this case given that the feature isn't yet fully 
> supported (I don't like the idea of setting a precedent for relying on the 
> ABI of incomplete features in general), but concepts is a sufficiently 
> important use case that I could imagine doing it as a one-off if needed.

This patch already extends `-fclang-abi-compat` to retain the old manglings so 
that users can stay on an old (broken) ABI if they need to. I don't think we 
need to do more than that for the concepts mangling changes.

>>> The ABI proposals haven't been accepted yet; I'm not intending to land this 
>>> change until the proposals have reached consensus in the Itanium C++ ABI 
>>> group.

The corresponding (confirmed) GCC bug report that they don't implement these 
mangling rules yet is: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100825

This patch supersedes https://reviews.llvm.org/D126818 (apologies for the 
duplicated work, @erichkeane -- I didn't find that before I started working on 
this).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

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

[PATCH] D147673: [Clang] Improve designated inits diagnostic location

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/test/SemaCXX/cxx2b-designated-initializers.cpp:13
+  const S result { // expected-error {{field designator (null) does not refer 
to any field in type 'const S'}}
+.a = x
+  };

Why are we rejecting this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147673/new/

https://reviews.llvm.org/D147673

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

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D147655#4248800 , @royjacobson 
wrote:

> I agree it doesn't affect too much code, but people do instantiate templates 
> manually sometimes. IIUC, linking against shared libraries would break if 
> that library does explicit instantiations of constrained functions. That 
> concerns me a bit.

There has not been any stable ABI from any compiler targeting the Itanium C++ 
ABI for constrained templates prior to this change. I don't think we need to 
worry too much about people using unfinished compiler features being broken 
when those features are finished.

From an ABI perspective, I think the other cases that I called out are more 
interesting than the constrained templates side of things: this changes the 
mangling for non-type template parameters with deduced types, non-type template 
parameters with instantiation-dependent types, and template template parameters 
that don't match their template template arguments, in the template parameter 
list of a function template. The latter two of those changes can in principle 
affect code dating back to C++98. There might be a case for putting those 
changes in particular behind a flag. @rjmccall I would particularly appreciate 
your feedback on that concern.

> Also, do you know if GCC developers are planning on implementing this change 
> as well?

I have no reason to think they would not follow the cross-vendor ABI 
specification, though I'll ping them explicitly. The ABI proposals haven't been 
accepted yet; I'm not intending to land this change until the proposals have 
reached consensus in the Itanium C++ ABI group.




Comment at: clang/include/clang/AST/ExprConcepts.h:502
ArrayRef LocalParameters,
+   SourceLocation RParenLoc,
ArrayRef Requirements,

erichkeane wrote:
> rsmith wrote:
> > erichkeane wrote:
> > > Is this an unrelated change?  Are paren locations required for mangling?
> > `requires (params) { blah; }` has a different mangling from `requires { 
> > blah; }` -- in particular, the former introduces a new level of function 
> > parameters, so references to enclosing function parameters are numbered 
> > differently in the two. This applies even if `params` is empty or `void` 
> > (`requires () { ... }` and `requires (void) { ... }` are both valid).
> > 
> > We previously generated the same AST for `requires { ... }` and `requires 
> > () { ... }`, which meant we couldn't mangle this correctly. Tracking the 
> > parens (or at least their existence) fixes that (and also improves the AST 
> > fidelity for tooling).
> >>Tracking the parens (or at least their existence) fixes tha
> it seems you test that via the `LParenLoc`, which is why I'm surprised about 
> the addition of the `RParenLoc`?  I have no problem with the addition of the 
> `RParenLoc` for the below reason, but was trying to figure out the 
> significance toward the mangling.
> 
> >>and also improves the AST fidelity for tooling
> This I definitely get, was just trying to figure out why it was in a patch 
> for mangling.
Yeah, we don't really need to track the `RParenLoc`, or the `LParenLoc` for 
that matter, as part of this mangling change, we just need any way to get the 
"were there parens?" information.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

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

[PATCH] D147744: [clang][Sema] Add MultiLevelTemplateArgumentList::dump()

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Sema/Template.h:265
+
+LLVM_DUMP_METHOD void dump() const {
+  LangOptions LO;

I think it would be useful to also print out the number of retained outer 
levels.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147744/new/

https://reviews.llvm.org/D147744

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

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D147722#4249612 , @erichkeane 
wrote:

> Hmmm... the examples I thought would cause a problem don't seem to when 
> switching that to Done.  So I'm going to run regression tests and give that a 
> shot.
>
> I think I now get what you mean about 'Identity' arguments, we basically need 
> to have arguments to keep something like the above example (where 'foo' is a 
> template, and the partial specialization is done correctly), and when trying 
> to substitute into it, we would end up having the arguments to `Outer` and 
> `Foo`, but substitution would think of the `Foo` args as the `Inner` args for 
> depth.  I'm giving the testing a shot, and will see if that fixes the problem 
> without causing a regression in any of the concerns I have.  I suspect I 
> don't have a problem with the 'identity' for the same reason 'Done' works: we 
> never hit that situation.

If we know that all further levels will be "identity" levels (mapping 
template-param-i-j to template-param-i-j), then we can use 
`MultiLevelTemplateArgumentList::addOuterRetainedLevels` to represent that and 
leave those levels alone. That will mean the `getNumSubstitutedLevels() == 0` 
check in `SubstituteConstraintExpression` will fire a lot more often, which 
seems like a nice benefit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147722/new/

https://reviews.llvm.org/D147722

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

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D146897#4249300 , @rjmccall wrote:

> The third idea seems like a valuable warning, but it's basically a 
> *different* warning and shouldn't be lumped into this one.  I understand the 
> instinct to carve it out here so that we don't regress on warning about it, 
> but I think we should just do it separately.  And we should do it much more 
> generally, because there's no reason that logic is limited to just these 
> specific compound operators, or in fact to any individual operator; we should 
> probably warn about all inconsistent references to the same variable within a 
> single statement.

Yeah, good point; I'm convinced.

> (I'd draw the line at statement boundaries, though, because requiring 
> function-wide consistency could be really noisy.)

At least, I don't think we should warn if both `a` and `this->a` are used in 
different statements in the same function, and `a` is shadowed by a local 
declaration wherever `this->a` is used, and I think it might be reasonable to 
not warn on a member `a` if the name `a` is ever declared in the function 
regardless of where it's in scope. For the remaining cases, where `a` is never 
declared locally, I suspect the false positive rate for warning if the function 
uses both `a` and `this->a` would be lower, but we could probably get some data 
on that pretty easily. Speaking of data, it'd be nice to get some evidence that 
this is a mistake that actually happens before landing a dedicated warning for 
it :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146897/new/

https://reviews.llvm.org/D146897

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

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2854-2858
-template <>
-bool DeducedArgsNeedReplacement(
-ClassTemplatePartialSpecializationDecl *Spec) {
-  return !Spec->isClassScopeExplicitSpecialization();
-}

It's not obvious to me what this was doing, so I'm not sure whether it's 
correct to remove it. Can you explain? It makes me uncomfortable that we would 
treat class template partial specializations and variable template partial 
specializations differently, at least without a good explanation for why that's 
appropriate -- perhaps we should apply this same set of changes to variable 
template partial specializations too, and remove this mechanism entirely?



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:318
+  // specialized template anyway, so any substitution we would do with 
these
+  // partially specialized arguments would 'wrong' and confuse constraint
+  // instantiation. We only do this in the case of a constraint check, 
since





Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:322
+  // they mean.
+  R = Response::UseNextDecl(PartialClassTemplSpec);
 } else if (const auto *ClassTemplSpec =

Can we produce a "done" response instead here? It doesn't seem right to carry 
on to the next level here without adding a level of template arguments -- if 
any further arguments were somehow collected, they would be collected at  the 
wrong template depth. But, as far as I can determine, we should never encounter 
a partial specialization declaration except in cases where there is nothing 
remaining to be substituted (all outer substitutions would be identity 
substitutions). I think the same thing applies when we reach a class template 
declaration (the first branch in `HandleRecordDecl`) -- we should just be able 
to stop in that case too, rather than collecting some identity template 
arguments and carrying on to an outer level.

If we can't produce a "done" response here for some reason, then we should 
collect a level of identity template arguments instead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147722/new/

https://reviews.llvm.org/D147722

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

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/AST/ExprConcepts.h:502
ArrayRef LocalParameters,
+   SourceLocation RParenLoc,
ArrayRef Requirements,

erichkeane wrote:
> Is this an unrelated change?  Are paren locations required for mangling?
`requires (params) { blah; }` has a different mangling from `requires { blah; 
}` -- in particular, the former introduces a new level of function parameters, 
so references to enclosing function parameters are numbered differently in the 
two. This applies even if `params` is empty or `void` (`requires () { ... }` 
and `requires (void) { ... }` are both valid).

We previously generated the same AST for `requires { ... }` and `requires () { 
... }`, which meant we couldn't mangle this correctly. Tracking the parens (or 
at least their existence) fixes that (and also improves the AST fidelity for 
tooling).



Comment at: clang/lib/AST/ItaniumMangle.cpp:5180
+NotPrimaryExpr();
+if (RE->getLParenLoc().isValid()) {
+  Out << "rQ";

erichkeane wrote:
> What is happening here? What are we deriving from the validity of the paren?  
> Or is this differentiating:
> 
> `requires (something)` vs `requires C` sorta thing?
Yes, the `rq` form is for requires expressions without a parameter list, and 
the `rQ` form is for requires expressions with a parameter list.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

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

[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I'm happy whenever Aaron is.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8809
+  //
+  // We will support P2448R2 in language modes earlier than C++23 as an 
extenion
+  // The concept of constexpr-compatible was removed.





Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8826-8827
+  getLangOpts().CPlusPlus2b
+  ? 
diag::warn_cxx2b_compat_incorrect_defaulted_comparison_constexpr
+  : diag::ext_incorrect_defaulted_comparison_constexpr)
   << FD->isImplicit() << (int)DCK << FD->isConsteval();

I wonder if `incorrect_` is the best way to spell the diagnostic ID given the 
new rule. Suggested an alternative.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146090/new/

https://reviews.llvm.org/D146090

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

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Here's the diff to `test_demangle.pass.cpp`:

  --- a/libcxxabi/test/test_demangle.pass.cpp
  +++ b/libcxxabi/test/test_demangle.pass.cpp
  @@ -29995,10 +29995,10 @@ const char* cases[][2] =
   {"_Z15test_uuidofTypeI10TestStructEvDTu8__uuidofT_EE", "void 
test_uuidofType(decltype(__uuidof(TestStruct)))"},
   {"_Z15test_uuidofExprI9HasMemberEvDTu8__uuidofXsrT_6memberEEE", "void 
test_uuidofExpr(decltype(__uuidof(HasMember::member)))"},
   
  -// C++2a char8_t:
  +// C++20 char8_t:
   {"_ZTSPDu", "typeinfo name for char8_t*"},
   
  -// C++2a lambda-expressions:
  +// C++20 lambda-expressions:
   {"_ZNK1xMUlTyT_E_clIiEEDaS_", "auto x::'lambda'($T)::operator()(x) const"},
   {"_ZNK1xMUlTnPA3_ivE_clILS0_0EEEDav", "auto x::'lambda'()::operator()<(int [3])0>() const"},
   {"_ZNK1xMUlTyTtTyTnT_TpTnPA3_TL0__ETpTyvE_clIi1XJfEEEDav", "auto 
x::'lambda' 
typename $TT, typename ...$T1>()::operator()() const"},
  @@ -30015,8 +30015,10 @@ const char* cases[][2] =
   // See https://github.com/itanium-cxx-abi/cxx-abi/issues/106.
   {"_ZN1XIZ1fIiEvOT_EUlS2_DpT0_E_EclIJEEEvDpT_", "void X(int&&)::'lambda'(int&&, auto...)>::operator()<>()"},
   
{"_N6abcdef9abcdefghi29abcdefabcdefabcdefabcefabcdef27xxxEN4absl8DurationERKNSt3__u12basic_stringIcNS4_11char_traitsIcEENS4_9allocatorIcPNS1_19yyyEENK3$_5clEvENKUlvE_clEvE6zz",
 
"abcdef::abcdefghi::abcdefabcdefabcdefabcefabcdef::xxx(absl::Duration,
 std::__u::basic_string, 
std::__u::allocator> const&, 
abcdef::abcdefghi::abcdefabcdefabcdefabcefabcdef::yyy*)::$_5::operator()()
 const::'lambda'()::operator()() const::zz"},
  +// See https://github.com/itanium-cxx-abi/cxx-abi/issues/165.
  +{"_ZN1C1fIiEEvDTtlNS_UlT_TL0__E_EEE", "void 
C::f(decltype(C::'lambda'(int, auto){}))"},
   
  -// C++2a class type non-type template parameters:
  +// C++20 class type non-type template parameters:
   {"_Z1fIXtl1BLPi0ELi1vv", "void f()"},
   {"_Z1fIXtl1BLPi32vv", "void f()"},
   {"_Z1fIXtl1BrcPiLi0vv", "void f(0)}>()"},
  @@ -30089,6 +30091,51 @@ const char* cases[][2] =
   {"_ZW1ML4Oink", "Oink@M"},
   {"_ZW1ML1fi", "f@M(int)"},
   
  +// C++20 concepts, see 
https://github.com/itanium-cxx-abi/cxx-abi/issues/24.
  +{"_Z2f0IiE1SIX1CIT_EEEv", "S> f0()"},
  +{"_ZN5test21AIiEF1fEzQ4TrueIT_E", "test2::A::friend f(...) requires 
True"},
  +{"_ZN5test2F1gIvEEvzQaa4TrueIT_E4TrueITL0__E", "void test2::friend 
g(...) requires True && True"},
  +{"_ZN5test21hIvEEvzQ4TrueITL0__E", "void test2::h(...) requires 
True"},
  +{"_ZN5test2F1iIvQaa4TrueIT_E4TrueITL0__EEEvz", "void test2::friend 
i(...)"},
  +{"_ZN5test21jIvQ4TrueITL0__EEEvz", "void test2::j(...)"},
  +{"_ZN5test2F1kITk4TruevQ4TrueIT_EEEvz", "void test2::friend 
k(...)"},
  +{"_ZN5test21lITk4TruevEEvz", "void test2::l(...)"},
  +{"_ZN5test31dITnDaLi0EEEvv", "void test3::d<0>()"},
  +{"_ZN5test31eITnDcLi0EEEvv", "void test3::e<0>()"},
  +{"_ZN5test31fITnDk1CLi0EEEvv", "void test3::f<0>()"},
  +{"_ZN5test31gITnDk1DIiELi0EEEvv", "void test3::g<0>()"},
  +{"_ZN5test31hIiTnDk1DIT_ELi0EEEvv", "void test3::h()"},
  +{"_ZN5test31iIiEEvDTnw_Dk1CpicvT__EEE", "void test3::i(decltype(new 
C auto((int)("},
  +{"_ZN5test31jIiEEvDTnw_DK1CpicvT__EEE", "void test3::j(decltype(new 
C decltype(auto)((int)("},
  +{"_ZN5test41fITk1CiEEvv", "void test4::f()"},
  +{"_ZN5test41gITk1DIiEiEEvv", "void test4::g()"},
  +{"_ZN5test51fINS_1XEEEvv", "void test5::f()"},
  +{"_ZN5test51fITtTyTnTL0__ENS_1YEEEvv", "void test5::f()"},
  +{"_ZN5test51fITtTyTnTL0__ENS_1ZEEEvv", "void test5::f()"},
  +{"_ZN5test51gITtTyTnTL0__Q1CIS1_EENS_1XEEEvv", "void 
test5::g()"},
  +{"_ZN5test51gINS_1YEEEvv", "void test5::g()"},
  +{"_ZN5test51gITtTyTnTL0__Q1CIS1_EENS_1ZEEEvv", "void 
test5::g()"},
  +{"_ZN5test51hITtTk1CTnTL0__ENS_1XEEEvv", "void test5::h()"},
  +{"_ZN5test51hITtTk1CTnTL0__ENS_1YEEEvv", "void test5::h()"},
  +{"_ZN5test51hINS_1ZEEEvv", "void test5::h()"},
  +{"_ZN5test51iITpTtTk1CTnTL0__EJNS_1XENS_1YENS_1Zvv", "void 
test5::i()"},
  +{"_ZN5test51iITpTtTk1CTnTL0__EJNS_1YENS_1ZENS_1Xvv", "void 
test5::i()"},
  +{"_ZN5test51iIJNS_1ZENS_1XENS_1Yvv", "void test5::i()"},
  +{"_ZN5test51pINS_1AEEEvv", "void test5::p()"},
  +{"_ZN5test51pITtTpTyENS_1BEEEvv", "void test5::p()"},
  +{"_ZN5test51qITtTyTyENS_1AEEEvv", "void test5::q()"},
  +{"_ZN5test51qINS_1BEEEvv", "void test5::q()"},
  +{"_ZN5test61fITk1CiEEvT_", "void test6::f(int)"},
  +{"_ZN5test61gIiTk1DIT_EiEEvT0_", "void test6::g(int)"},
  +
{"_ZZN5test71fIiEEvvENKUlTyQaa1CIT_E1CITL0__ET0_E_clIiiEEDaS3_Q1CIDtfp_EE", 
"auto void test7::f()::'lambda' requires C && C 
(auto)::operator()(auto) const requires C"},
  +

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15656
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;

python3kgae wrote:
> rsmith wrote:
> > This is the same thing, but for `this->x += this->x`. I think we should 
> > also suppress those warnings for compound assignment, except when one of 
> > the operands is an implicit member access and one is an explicit member 
> > access (`this->x += x;` should still warn if the latter `x` is also 
> > interpreted as `this->x`).
> For case like this:
> 
> ```
> class Vector {
> public:
> Vector& operator+=(const Vector ) { return *this; }
> Vector& operator-=(const Vector ) { return *this; }
> };
> 
> class A {
> public:
>   A(){}
>   Vector x;
>   void foo() {
>  this->x -= this->x;
>  this->x -= x;
>  this->x += this->x;
>  this->x += x;
>   }
> };
> ```
> clang will report 2 warning:
> 
> ```
> :14:14: warning: assigning field to itself [-Wself-assign-field]
>  this->x -= this->x;
>  ^
> :15:14: warning: assigning field to itself [-Wself-assign-field]
>  this->x -= x;
> ```
> 
> Do you mean we should make it report warning on this->x -= x; and this->x += 
> x; ?
Let's not add any new warnings. Pretend my comment was about `-=`, not `+=`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146897/new/

https://reviews.llvm.org/D146897

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

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15656
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;

This is the same thing, but for `this->x += this->x`. I think we should also 
suppress those warnings for compound assignment, except when one of the 
operands is an implicit member access and one is an explicit member access 
(`this->x += x;` should still warn if the latter `x` is also interpreted as 
`this->x`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146897/new/

https://reviews.llvm.org/D146897

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Does my horrible lambda example work now? If so, that seems like a useful 
testcase.




Comment at: clang/lib/Sema/SemaConcept.cpp:781
+  /*ForConstraintInstantiation=*/true, /*SkipForSpecialization*/ false);
+  Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/false);
+  std::optional ThisScope;

Just a small optimization: there's no point doing the transform if we have 
nothing to substitute.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146178/new/

https://reviews.llvm.org/D146178

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

[PATCH] D147068: Fix merging of member-like constrained friends across modules.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith closed this revision.
rsmith added a comment.

Refactoring and test landed in rGa07abe27b4d1d39ebb940a7f4e6235302444cbf0 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147068/new/

https://reviews.llvm.org/D147068

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:775-776
+static bool IsInsideImplicitFullTemplateSpecialization(const DeclContext *DC) {
+  auto *CTSDecl = dyn_cast_or_null(
+  DC->getOuterLexicalRecordContext());
+  return CTSDecl && !isa(CTSDecl) &&

rsmith wrote:
> This doesn't look right to me; there could be a class template nested inside 
> a non-templated class, so I think you would need to walk up the enclosing 
> `DeclContext`s one by one checking each in turn. But, we might be inside a 
> function template specialization or variable template specialization instead, 
> in some weird cases:
> 
> ```
> template concept C = true;
> template auto L = [] U>() {};
> struct Q {
>   template U> friend constexpr auto decltype(L)::operator()() 
> const;
> };
> ```
> 
> ... so I think we want a different approach than looking for an enclosing 
> class template specialization declaration.
> 
> Can we skip this check entirely, and instead always compute and substitute 
> the template instantiation arguments as is done below? That computation will 
> walk the enclosing contexts for us in a careful way that properly handles 
> cases like this lambda-in-variable-template situation. If we find we get zero 
> levels of template argument list, we can skip doing the actual substitution 
> as an optimization.
Sorry, that should be:
```
template concept C = true;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146178/new/

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Can you also change www/cxx_status.html to list P2103R0 as supported in the 
most recent version of Clang rather than in some previous version?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146178/new/

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:775-776
+static bool IsInsideImplicitFullTemplateSpecialization(const DeclContext *DC) {
+  auto *CTSDecl = dyn_cast_or_null(
+  DC->getOuterLexicalRecordContext());
+  return CTSDecl && !isa(CTSDecl) &&

This doesn't look right to me; there could be a class template nested inside a 
non-templated class, so I think you would need to walk up the enclosing 
`DeclContext`s one by one checking each in turn. But, we might be inside a 
function template specialization or variable template specialization instead, 
in some weird cases:

```
template concept C = true;
template auto L = [] U>() {};
struct Q {
  template U> friend constexpr auto decltype(L)::operator()() const;
};
```

... so I think we want a different approach than looking for an enclosing class 
template specialization declaration.

Can we skip this check entirely, and instead always compute and substitute the 
template instantiation arguments as is done below? That computation will walk 
the enclosing contexts for us in a careful way that properly handles cases like 
this lambda-in-variable-template situation. If we find we get zero levels of 
template argument list, we can skip doing the actual substitution as an 
optimization.



Comment at: clang/lib/Sema/SemaOverload.cpp:1303
+OldTemplate->getTemplateParameters(), false, TPL_TemplateMatch,
+SourceLocation(), false /* PartialOrdering */);
 bool SameReturnType = Context.hasSameType(Old->getDeclaredReturnType(),

rsmith wrote:
> shafik wrote:
> > nit
> Just remove the final parameter; it has a default argument of `false` and no 
> other call site passes `false` here.  (I'm working on removing this parameter 
> in a different change.)
You can remove the `SourceLocation()` argument too; there's an identical 
default argument.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146178/new/

https://reviews.llvm.org/D146178

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

[PATCH] D147281: Stop modifying trailing return types.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGaeee4ebd6891: Stop modifying trailing return types. 
(authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D147281?vs=509840=510321#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147281/new/

https://reviews.llvm.org/D147281

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaConcept.cpp


Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -704,26 +704,10 @@
 Record = const_cast(Method->getParent());
   }
   CXXThisScopeRAII ThisScope(*this, Record, ThisQuals, Record != nullptr);
-  // We substitute with empty arguments in order to rebuild the atomic
-  // constraint in a constant-evaluated context.
-  // FIXME: Should this be a dedicated TreeTransform?
-  const Expr *RC = FD->getTrailingRequiresClause();
-  llvm::SmallVector Converted;
-
-  if (CheckConstraintSatisfaction(
-  FD, {RC}, Converted, *MLTAL,
-  SourceRange(UsageLoc.isValid() ? UsageLoc : FD->getLocation()),
-  Satisfaction))
-return true;
-
-  // FIXME: we need to do this for the function constraints for
-  // comparison of constraints to work, but do we also need to do it for
-  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we
-  // seem to always just pick up the constraints from the primary template.
-  assert(Converted.size() <= 1 && "Got more expressions converted?");
-  if (!Converted.empty() && Converted[0] != nullptr)
-const_cast(FD)->setTrailingRequiresClause(Converted[0]);
-  return false;
+  return CheckConstraintSatisfaction(
+  FD, {FD->getTrailingRequiresClause()}, *MLTAL,
+  SourceRange(UsageLoc.isValid() ? UsageLoc : FD->getLocation()),
+  Satisfaction);
 }
 
 
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6619,28 +6619,8 @@
   return false;
 }
 
-// The trailing require clause of instantiated function may change during
-// the semantic analysis. Trying to get the primary template function (if
-// exists) to compare the primary trailing require clause.
-auto TryToGetPrimaryTemplatedFunction =
-[](const FunctionDecl *FD) -> const FunctionDecl * {
-  switch (FD->getTemplatedKind()) {
-  case FunctionDecl::TK_DependentNonTemplate:
-return FD->getInstantiatedFromDecl();
-  case FunctionDecl::TK_FunctionTemplate:
-return FD->getDescribedFunctionTemplate()->getTemplatedDecl();
-  case FunctionDecl::TK_MemberSpecialization:
-return FD->getInstantiatedFromMemberFunction();
-  case FunctionDecl::TK_FunctionTemplateSpecialization:
-return FD->getPrimaryTemplate()->getTemplatedDecl();
-  default:
-return FD;
-  }
-};
-const FunctionDecl *PrimaryX = TryToGetPrimaryTemplatedFunction(FuncX);
-const FunctionDecl *PrimaryY = TryToGetPrimaryTemplatedFunction(FuncY);
-if (!isSameConstraintExpr(PrimaryX->getTrailingRequiresClause(),
-  PrimaryY->getTrailingRequiresClause()))
+if (!isSameConstraintExpr(FuncX->getTrailingRequiresClause(),
+  FuncY->getTrailingRequiresClause()))
   return false;
 
 auto GetTypeAsWritten = [](const FunctionDecl *FD) {


Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -704,26 +704,10 @@
 Record = const_cast(Method->getParent());
   }
   CXXThisScopeRAII ThisScope(*this, Record, ThisQuals, Record != nullptr);
-  // We substitute with empty arguments in order to rebuild the atomic
-  // constraint in a constant-evaluated context.
-  // FIXME: Should this be a dedicated TreeTransform?
-  const Expr *RC = FD->getTrailingRequiresClause();
-  llvm::SmallVector Converted;
-
-  if (CheckConstraintSatisfaction(
-  FD, {RC}, Converted, *MLTAL,
-  SourceRange(UsageLoc.isValid() ? UsageLoc : FD->getLocation()),
-  Satisfaction))
-return true;
-
-  // FIXME: we need to do this for the function constraints for
-  // comparison of constraints to work, but do we also need to do it for
-  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we
-  // seem to always just pick up the constraints from the primary template.
-  assert(Converted.size() <= 1 && "Got more expressions converted?");
-  if (!Converted.empty() && Converted[0] != nullptr)
-const_cast(FD)->setTrailingRequiresClause(Converted[0]);
-  return false;
+  return CheckConstraintSatisfaction(
+  FD, {FD->getTrailingRequiresClause()}, *MLTAL,
+  SourceRange(UsageLoc.isValid() ? UsageLoc : 

[PATCH] D147068: Fix merging of member-like constrained friends across modules.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Hm, looks like rGdb1b827ecfed6 
 landed 
the actual functioal change here, making this just a no-functional-change 
refactoring plus an added test, so I'm going to go ahead and land it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147068/new/

https://reviews.llvm.org/D147068

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