[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

From a purely personal perspective, I'd prefer if this landed after the branch 
for llvm-15.

We try to co-release IWYU shortly after LLVM/Clang are released, to get a 
public API-compatible release out there. So it would be really nice if we 
didn't have to spend time working around AST changes while the clock is ticking 
to get a release out the door.

That said, I know we're just an out-of-tree project and the LLVM project as 
such doesn't make any promises (and shouldn't have to!). I just thought I'd 
raise the concern to see if anybody shares it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

In D112374#3657640 , @mizvekov wrote:

> In D112374#3657472 , @kimgr wrote:
>
>> I'm coming at this from pretty far away, so there's very likely lots of 
>> details that I'm overlooking. But it seems to me the mainline had only had 
>> an `ElaboratedType` node if there was elaboration, and not otherwise. And 
>> that makes a lot more sense to me than having 2 `ElaboratedType*` nodes _for 
>> every type in the AST_, just to express that "hey, by the way, this type had 
>> no elaboration".
>
> There are no 2 `ElaboratedType` nodes, there is only one. If you are seeing 
> something like an ElaboratedType wrapping directly over another 
> ElaboratedType, that would seem to be a bug.

I meant the `ElaboratedTypeLoc` + `ElaboratedType`, but yeah, those are 
parallel, not nested.

>> That sounds good at face value, but if you're planning to remove these nodes 
>> again, that would create enormous churn for out-of-tree tools to re-adjust 
>> to the new shape of the tree.
>>
>> I can't say what the best solution is, but this patch generates quite a lot 
>> of work for me, and I would really hope that catching up with the new AST 
>> does not generate even more work down the line.
>
> That part I don't understand why. Before this patch, clang can produce a 
> bunch of type nodes wrapped in an ElTy, or not. After this patch, we add 
> ElTys in more cases, but the basic situation remains the same.
>
> Why IWYU would even care about ElaboratedTypes at all? I would have expected 
> a `git grep ElaboratedType` on IWYU sources to have no matches.
>
> Can you elaborate on that?

Haha. Pun intended? :-)

> In general, I would not expect external tools to care about the shape of the 
> AST. I would expect the type API would be used in a way where we ignore a 
> type sugar node we have no reason to acknowledge.
> Ie you query if some type is a (possible sugar to) X, and you would either 
> get X or nothing. The type sugar over it would just be skipped over and you 
> would have no reason to know what was in there or what shape it had.
>
> Of course that was not what happened in practice. A lot of code used 
> `dyn_cast` where it should have used `getAs`. Just look at all the fixes in 
> this patch for examples.
> And fixing that, besides making that code compatible with this patch, also 
> fixed other bugs where it would not properly ignore other pre-existing type 
> sugar.

As you noticed, it's not our tests that care about the AST, it's the tool 
itself. IWYU has been around since 2010-11, so there's probably lots of code in 
there to work around bugs and idiosyncrasies in the Clang AST that have since 
been fixed. I've inherited the project, so I don't have much information on how 
or why the implementation ended up the way it did.

Anyway, thanks for the heads-up about `getAs` vs `dyn_cast`, that seems like an 
easy transformation for us to do. Though I'm not sure where -- everywhere a 
`Type*` is processed?

Also, I suspect we have a few open-ended searches where we're looking for the 
first desugared type in the tree, but I guess that's where `getCanonicalType` 
would be used?

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

> It's the difference in knowing the type was written without any tag or 
> nested-name specifier, and having a type that you are not sure how it was 
> written.
>
> When we are dealing with a type which we are not sure, we would like to print 
> it fully qualified, with a synthetic nested name specifier computed from it's 
> DC,
> because otherwise it could be confusing as the type could come from somewhere 
> very distant from the context we are printing the type from. We would not
> want to assume that a type which has been desugared was written how it's 
> desugared state would seem to imply.

I'm coming at this from pretty far away, so there's very likely lots of details 
that I'm overlooking. But it seems to me the mainline had only had an 
`ElaboratedType` node if there was elaboration, and not otherwise. And that 
makes a lot more sense to me than having 2 `ElaboratedType*` nodes _for every 
type in the AST_, just to express that "hey, by the way, this type had no 
elaboration".

> FWIW, in the state of affairs we leave clang after this patch, I don't think 
> it's worth keeping a separate ElaboratedType anymore, we might as 
> well fuse it's functionality into the type nodes which could be wrapped in 
> it. Taking care to optimize storage when not used otherwise, I think
> we can recoup the performance lost in this patch, perhaps even end in a 
> better state overall.
>
> But I think doing these two steps in one go would not be sensibly 
> incremental. We have in this patch here a very simple core change, which
> is very unlikely to have bugs in itself, but creates enormous test churn.
>
> The second step of eliminating ElaboratedType could be a less simple core 
> change with almost zero test churn, which makes it less risky that
> it would introduce a bug that escapes review.

That sounds good at face value, but if you're planning to remove these nodes 
again, that would create enormous churn for out-of-tree tools to re-adjust to 
the new shape of the tree.

I can't say what the best solution is, but this patch generates quite a lot of 
work for me, and I would really hope that catching up with the new AST does not 
generate even more work down the line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

It's a little confusing, because it now looks like _every_ `Type` in the AST is 
wrapped in an `ElaboratedTypeLoc` + `ElaboratedType`. IWYU's debug AST dump 
shows this (excerpt):

  tests/cxx/sizeof_reference.cc:51:8: (1) [ VarDecl ] size_t s2 

 
  tests/cxx/sizeof_reference.cc:51:1: (2) [ ElaboratedTypeLoc ] size_t  

 
  tests/cxx/sizeof_reference.cc:51:1: (2) [ ElaboratedType ] size_t 

 
  tests/cxx/sizeof_reference.cc:51:1: (3) [ TypedefTypeLoc ] size_t 

 
  tests/cxx/sizeof_reference.cc:51:1: (3) [ TypedefType ] size_t

 
  Marked full-info use of decl size_t (from 
/home/kimgr/code/iwyu/out/main/lib/clang/15.0.0/include/stddef.h:46:23) at 
tests/cxx/sizeof_reference.cc:51:1
  tests/cxx/sizeof_reference.cc:51:13: (2) [ UnaryExprOrTypeTraitExpr ] 
UnaryExprOrTypeTraitExpr 0x5589fd2a4230 'unsigned long' sizeof 
'IndirectTemplateStruct &' 


 
  (For type IndirectTemplateStruct): 

 
  Marked full-info use of decl IndirectTemplateStruct (from 
tests/cxx/sizeof_reference.cc:18:30) at tests/cxx/sizeof_reference.cc:51:20 
 
  tests/cxx/sizeof_reference.cc:51:20: (3) [ LValueReferenceTypeLoc ] 
IndirectTemplateStruct & 
   
  tests/cxx/sizeof_reference.cc:51:20: (3) [ LValueReferenceType ] 
IndirectTemplateStruct & 
  
  tests/cxx/sizeof_reference.cc:51:20: (4) [ ElaboratedTypeLoc ] 
IndirectTemplateStruct   

  tests/cxx/sizeof_reference.cc:51:20: (4) [ ElaboratedType ] 
IndirectTemplateStruct   
   
  tests/cxx/sizeof_reference.cc:51:20: (5) [ TemplateSpecializationTypeLoc ] 
IndirectTemplateStruct   

  tests/cxx/sizeof_reference.cc:51:20: (5) [ TemplateSpecializationType ] 
IndirectTemplateStruct   
   
  Marked fwd-decl use of decl IndirectTemplateStruct (from 
tests/cxx/sizeof_reference.cc:18:30) at tests/cxx/sizeof_reference.cc:51:20 
  
  tests/cxx/sizeof_reference.cc:51:20: (6, fwd decl) [ TemplateName ] 
IndirectTemplateStruct  
   
  tests/cxx/sizeof_reference.cc:51:43: (6, fwd decl) [ TemplateArgumentLoc ] 
 

  tests/cxx/sizeof_reference.cc:51:43: (7, fwd decl) [ ElaboratedTypeLoc ] 
IndirectClass   
  
  tests/cxx/sizeof_reference.cc:51:43: (7, fwd decl) [ ElaboratedType ] 
IndirectClass   
 
  tests/cxx/sizeof_reference.cc:51:43: (8, fwd decl) [ RecordTypeLoc ] class 
IndirectClass   

  tests/cxx/sizeof_reference.cc:51:43: (8, fwd decl) [ RecordType ] class 
IndirectClass   
   
  Marked fwd-decl 

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

This patch also broke IWYU, not exactly sure how or why yet. We run around the 
AST quite a bit, so structural changes like this often bite us.

Can you expand on what happened here? Before/after kind-of thing? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr closed this revision.
kimgr added a comment.
Herald added a project: All.

Now that D119477  has landed, this suggested 
change is obsolete. Closing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D119476: Generalize and harmonize sub-expression traversal

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Thanks! Could you help me land it? Author: Kim Gräsman .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119476

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

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

[Kim Gräsman via Phabricator via cfe-commits]

kimgr updated this revision to Diff 416695.
kimgr added a comment.

Address review feedback

- Restore accidentally removed test line
- Clarify diagnostic expectations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119477

Files:
  clang/lib/AST/Expr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp
  clang/unittests/Tooling/CastExprTest.cpp

Index: clang/unittests/Tooling/CastExprTest.cpp
===
--- clang/unittests/Tooling/CastExprTest.cpp
+++ clang/unittests/Tooling/CastExprTest.cpp
@@ -14,12 +14,19 @@
 
 struct CastExprVisitor : TestVisitor {
   std::function OnExplicitCast;
+  std::function OnCast;
 
   bool VisitExplicitCastExpr(ExplicitCastExpr *Expr) {
 if (OnExplicitCast)
   OnExplicitCast(Expr);
 return true;
   }
+
+  bool VisitCastExpr(CastExpr *Expr) {
+if (OnCast)
+  OnCast(Expr);
+return true;
+  }
 };
 
 TEST(CastExprTest, GetSubExprAsWrittenThroughMaterializedTemporary) {
@@ -54,4 +61,57 @@
   CastExprVisitor::Lang_CXX2a);
 }
 
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// constructor conversions (https://github.com/llvm/llvm-project/issues/53044):
+//
+// `-ImplicitCastExpr 'X' 
+//   `-ConstantExpr 'X'
+// |-value: Struct
+// `-CXXConstructExpr 'X' 'void (const char *)'
+//   `-ImplicitCastExpr 'const char *' 
+// `-StringLiteral 'const char [7]' lvalue "foobar"
+TEST(CastExprTest, GetCtorConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+if (Expr->getCastKind() == CK_ConstructorConversion) {
+  auto *Conv = Expr->getConversionFunction();
+  EXPECT_TRUE(isa(Conv))
+  << "Expected CXXConstructorDecl, but saw " << Conv->getDeclKindName();
+}
+  };
+  Visitor.runOver("struct X { consteval X(const char *) {} };\n"
+  "void f() { X x = \"foobar\"; }\n",
+  CastExprVisitor::Lang_CXX2a);
+}
+
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// user-defined conversions.
+//
+// `-ImplicitCastExpr 'const char *' 
+//   `-ConstantExpr 'const char *'
+// |-value: LValue
+// `-CXXMemberCallExpr 'const char *'
+//   `-MemberExpr '' .operator const char *
+// `-DeclRefExpr 'const X' lvalue Var 'x' 'const X'
+TEST(CastExprTest, GetUserDefinedConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+if (Expr->getCastKind() == CK_UserDefinedConversion) {
+  auto *Conv = Expr->getConversionFunction();
+  EXPECT_TRUE(isa(Conv))
+  << "Expected CXXMethodDecl, but saw " << Conv->getDeclKindName();
+}
+  };
+  Visitor.runOver("struct X {\n"
+  "  consteval operator const char *() const {\n"
+  "return nullptr;\n"
+  "  }\n"
+  "};\n"
+  "const char *f() {\n"
+  "  constexpr X x;\n"
+  "  return x;\n"
+  "}\n",
+  CastExprVisitor::Lang_CXX2a);
+}
+
 } // namespace
Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -359,22 +359,34 @@
   // expected-note@-1 {{is not a constant expression}}
   { A k = to_lvalue_ref(A()); } // expected-error {{is not a constant expression}}
   // expected-note@-1 {{is not a constant expression}} expected-note@-1 {{temporary created here}}
-  { A k = to_lvalue_ref(A().ret_a()); } // expected-error {{is not a constant expression}}
-  // expected-note@-1 {{is not a constant expression}} expected-note@-1 {{temporary created here}}
+  { A k = to_lvalue_ref(A().ret_a()); }
+  // expected-error@-1 {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-2 {{heap-allocated object is not a constant expression}}
+  // expected-error@-3 {{'alloc::to_lvalue_ref' is not a constant expression}}
+  // expected-note@-4 {{reference to temporary is not a constant expression}}
+  // expected-note@-5 {{temporary created here}}
   { int k = A().ret_a().ret_i(); }
+  // expected-error@-1 {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-2 {{heap-allocated object is not a constant expression}}
   { int k = by_value_a(A()); }
   { int k = const_a_ref(A()); }
   { int k = const_a_ref(a); }
   { int k = rvalue_ref(A()); }
   { int k = rvalue_ref(std::move(a)); }
   { int k = const_a_ref(A().ret_a()); }
+  // expected-error@-1 {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-2 {{is not a constant expression}}
   { int k = const_a_ref(to_lvalue_ref(A().ret_a())); }
+  // expected-error@-1 {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-2 {{is not a 

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:362-364
+  { A k = to_lvalue_ref(A().ret_a()); } // expected-error {{'alloc::A::ret_a' 
is not a constant expression}} expected-error {{'alloc::to_lvalue_ref' is not a 
constant expression}}
+  // expected-note@-1 {{temporary created here}}
+  // expected-note@-2 {{heap-allocated object is not a constant expression}} 
expected-note@-2 {{reference to temporary is not a constant expression}}

aaron.ballman wrote:
> I usually prefer line continuation characters because I think it makes the 
> test easier to read (it's easy to miss secondary diagnostics on the same 
> line). However, I don't insist on these changes either (but if you make them, 
> please do similar for the other test lines you're touching).
Thanks, I wasn't aware there was support for line continuation. I agree it 
would benefit readability here, so I'll look into it. 



Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:365
-  { int k = A().ret_a().ret_i(); }
-  { int k = by_value_a(A()); }
   { int k = const_a_ref(A()); }

aaron.ballman wrote:
> Why are we dropping this test coverage?
Good question, that must've been a mistake. I'll take another look. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119477

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

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@aaron.ballman Friendly Monday ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119477

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

[PATCH] D119476: Generalize and harmonize sub-expression traversal

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@aaron.ballman Friendly Monday ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119476

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

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.
Herald added a project: All.

Gentle weekly ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119477

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

[PATCH] D119476: Generalize and harmonize sub-expression traversal

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.
Herald added a project: All.

Gentle weekly ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119476

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

[PATCH] D119476: Generalize and harmonize sub-expression traversal

[Kim Gräsman via Phabricator via cfe-commits]

kimgr updated this revision to Diff 411666.
kimgr added a comment.

Fix typo in comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119476

Files:
  clang/lib/AST/Expr.cpp


Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1899,51 +1899,48 @@
 }
 
 namespace {
-  const Expr *skipImplicitTemporary(const Expr *E) {
-// Skip through reference binding to temporary.
-if (auto *Materialize = dyn_cast(E))
-  E = Materialize->getSubExpr();
+// Skip over implicit nodes produced as part of semantic analysis.
+// Designed for use with IgnoreExprNodes.
+Expr *ignoreImplicitSemaNodes(Expr *E) {
+  if (auto *Materialize = dyn_cast(E))
+return Materialize->getSubExpr();
 
-// Skip any temporary bindings; they're implicit.
-if (auto *Binder = dyn_cast(E))
-  E = Binder->getSubExpr();
+  if (auto *Binder = dyn_cast(E))
+return Binder->getSubExpr();
 
-return E;
-  }
+  return E;
 }
+} // namespace
 
 Expr *CastExpr::getSubExprAsWritten() {
   const Expr *SubExpr = nullptr;
-  const CastExpr *E = this;
-  do {
-SubExpr = skipImplicitTemporary(E->getSubExpr());
+
+  for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) {
+SubExpr = IgnoreExprNodes(E->getSubExpr(), ignoreImplicitSemaNodes);
 
 // Conversions by constructor and conversion functions have a
 // subexpression describing the call; strip it off.
-if (E->getCastKind() == CK_ConstructorConversion)
-  SubExpr =
-
skipImplicitTemporary(cast(SubExpr->IgnoreImplicit())->getArg(0));
-else if (E->getCastKind() == CK_UserDefinedConversion) {
+if (E->getCastKind() == CK_ConstructorConversion) {
+  SubExpr = IgnoreExprNodes(
+  cast(SubExpr->IgnoreImplicit())->getArg(0),
+  ignoreImplicitSemaNodes);
+} else if (E->getCastKind() == CK_UserDefinedConversion) {
   SubExpr = SubExpr->IgnoreImplicit();
-  assert((isa(SubExpr) ||
-  isa(SubExpr)) &&
+  assert((isa(SubExpr) || isa(SubExpr)) &&
  "Unexpected SubExpr for CK_UserDefinedConversion.");
   if (auto *MCE = dyn_cast(SubExpr))
 SubExpr = MCE->getImplicitObjectArgument();
 }
+  }
 
-// If the subexpression we're left with is an implicit cast, look
-// through that, too.
-  } while ((E = dyn_cast(SubExpr)));
-
-  return const_cast(SubExpr);
+  return const_cast(SubExpr);
 }
 
 NamedDecl *CastExpr::getConversionFunction() const {
   const Expr *SubExpr = nullptr;
 
   for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) {
-SubExpr = skipImplicitTemporary(E->getSubExpr());
+SubExpr = IgnoreExprNodes(E->getSubExpr(), ignoreImplicitSemaNodes);
 
 if (E->getCastKind() == CK_ConstructorConversion)
   return cast(SubExpr)->getConstructor();


Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1899,51 +1899,48 @@
 }
 
 namespace {
-  const Expr *skipImplicitTemporary(const Expr *E) {
-// Skip through reference binding to temporary.
-if (auto *Materialize = dyn_cast(E))
-  E = Materialize->getSubExpr();
+// Skip over implicit nodes produced as part of semantic analysis.
+// Designed for use with IgnoreExprNodes.
+Expr *ignoreImplicitSemaNodes(Expr *E) {
+  if (auto *Materialize = dyn_cast(E))
+return Materialize->getSubExpr();
 
-// Skip any temporary bindings; they're implicit.
-if (auto *Binder = dyn_cast(E))
-  E = Binder->getSubExpr();
+  if (auto *Binder = dyn_cast(E))
+return Binder->getSubExpr();
 
-return E;
-  }
+  return E;
 }
+} // namespace
 
 Expr *CastExpr::getSubExprAsWritten() {
   const Expr *SubExpr = nullptr;
-  const CastExpr *E = this;
-  do {
-SubExpr = skipImplicitTemporary(E->getSubExpr());
+
+  for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) {
+SubExpr = IgnoreExprNodes(E->getSubExpr(), ignoreImplicitSemaNodes);
 
 // Conversions by constructor and conversion functions have a
 // subexpression describing the call; strip it off.
-if (E->getCastKind() == CK_ConstructorConversion)
-  SubExpr =
-skipImplicitTemporary(cast(SubExpr->IgnoreImplicit())->getArg(0));
-else if (E->getCastKind() == CK_UserDefinedConversion) {
+if (E->getCastKind() == CK_ConstructorConversion) {
+  SubExpr = IgnoreExprNodes(
+  cast(SubExpr->IgnoreImplicit())->getArg(0),
+  ignoreImplicitSemaNodes);
+} else if (E->getCastKind() == CK_UserDefinedConversion) {
   SubExpr = SubExpr->IgnoreImplicit();
-  assert((isa(SubExpr) ||
-  isa(SubExpr)) &&
+  assert((isa(SubExpr) || isa(SubExpr)) &&
  "Unexpected SubExpr for CK_UserDefinedConversion.");
   if (auto *MCE = dyn_cast(SubExpr))
 SubExpr = 

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I have a local branch with both D119476  and 
this patch.

- Rebased on latest main (853ca5472314e109b98e46f0985f27f79e17d2bd 
)
- Ran `ninja check-clang` and `ninja tools/clang/unittests/Tooling/ToolingTests 
&& tools/clang/unittests/Tooling/ToolingTests` without issue

I did some integration testing with D119095  
and could not find any conflict. More detail on that differential.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119477

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

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Having looked at this some more, I wonder if this patch is covering for another 
problem. Running even the simplest example fails on an assertion in 
`Sema::BuildCXXTypeConstructExpr`: https://godbolt.org/z/fMPcsh4f3.

It looks like that function is creating the majority of 
`CXXFunctionalCastExpr`s, so the way the patch shaves off 
`CXXFunctionalCastExpr` might be compensating for something that should never 
happen. I don't know much about this, so I can't say for sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119095

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

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I can also confirm that `main` and my branch (containing 
https://reviews.llvm.org/D119477 and https://reviews.llvm.org/D119476) both 
assert on the same line:

  $ cat bug-53244.cc 
  struct A {   
  consteval A() {}
  A(const A&) = delete;
  consteval void f() {}
  };
  
  int main() {
  A{A{}}.f();
  }
  
  $ bin/clang -std=c++20 ../../bug-53244.cc
  clang-14: /home/kimgr/code/llvm-project/clang/lib/Sema/SemaExprCXX.cpp:1453: 
clang::ExprResult clang::Sema::BuildCXXTypeConstructExpr(clang::TypeSourceInfo 
*, clang::SourceLocation, clang::MultiExprArg, clang::SourceLocation, bool): 
Assertion `(!ListInitialization || (Exprs.size() == 1 && 
isa(Exprs[0]))) && "List initialization must have initializer 
list as expression."' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.  Program arguments: 
/home/kimgr/code/llvm-project/out/main/bin/clang-14 -cc1 -triple 
x86_64-unknown-linux-gnu -emit-obj -mrelax-all --mrelax-relocations 
-disable-free -clear-ast-before-backend -main-file-name bug-53244.cc 
-mrelocation-model static -mframe-pointer=all -fmath-errno -ffp-contract=on 
-fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu x86-64 
-tune-cpu generic -mllvm -treat-scalable-fixed-error-as-warning 
-debugger-tuning=gdb 
-fcoverage-compilation-dir=/home/kimgr/code/llvm-project/out/main -resource-dir 
/home/kimgr/code/llvm-project/out/main/lib/clang/15.0.0 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/x86_64-linux-gnu/c++/9 
-internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/backward 
-internal-isystem 
/home/kimgr/code/llvm-project/out/main/lib/clang/15.0.0/include 
-internal-isystem /usr/local/include -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../x86_64-linux-gnu/include 
-internal-externc-isystem /usr/include/x86_64-linux-gnu 
-internal-externc-isystem /include -internal-externc-isystem /usr/include 
-std=c++20 -fdeprecated-macro 
-fdebug-compilation-dir=/home/kimgr/code/llvm-project/out/main -ferror-limit 19 
-fgnuc-version=4.2.1 -fno-implicit-modules -fcxx-exceptions -fexceptions 
-fcolor-diagnostics -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o 
/tmp/bug-53244-192e65.o -x c++ ../../bug-53244.cc
  1.   parser at end of file
  2.  ../../bug-53244.cc:7:12: parsing function body 'main'
   #0 0x0698e43a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
/home/kimgr/code/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:11
   #1 0x0698e5eb PrintStackTraceSignalHandler(void*) 
/home/kimgr/code/llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1
   #2 0x0698cc9a llvm::sys::RunSignalHandlers() 
/home/kimgr/code/llvm-project/llvm/lib/Support/Signals.cpp:97:5
   #3 0x0698ed15 SignalHandler(int) 
/home/kimgr/code/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
   #4 0x7f8769c493c0 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x153c0)
   #5 0x7f87696dd18b raise 
/build/glibc-eX1tMB/glibc-2.31/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
   #6 0x7f87696bc859 abort 
/build/glibc-eX1tMB/glibc-2.31/stdlib/abort.c:81:7
   #7 0x7f87696bc729 get_sysdep_segment_value 
/build/glibc-eX1tMB/glibc-2.31/intl/loadmsgcat.c:509:8
   #8 0x7f87696bc729 _nl_load_domain 
/build/glibc-eX1tMB/glibc-2.31/intl/loadmsgcat.c:970:34
   #9 0x7f87696cdf36 (/lib/x86_64-linux-gnu/libc.so.6+0x36f36)
  #10 0x0af7d719 
clang::Sema::BuildCXXTypeConstructExpr(clang::TypeSourceInfo*, 
clang::SourceLocation, llvm::MutableArrayRef, 
clang::SourceLocation, bool) 
/home/kimgr/code/llvm-project/clang/lib/Sema/SemaExprCXX.cpp:1454:39
  #11 0x0ade829d 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 
>*>)::ComplexRemove>::RebuildCXXFunctionalCastExpr(clang::TypeSourceInfo*, 
clang::SourceLocation, clang::Expr*, clang::SourceLocation, bool) 
/home/kimgr/code/llvm-project/clang/lib/Sema/TreeTransform.h:3019:22
  #12 0x0adac3d5 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 
>*>)::ComplexRemove>::TransformCXXFunctionalCastExpr(clang::CXXFunctionalCastExpr*)
 /home/kimgr/code/llvm-project/clang/lib/Sema/TreeTransform.h:11728:23
  #13 0x0ada4a8f 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 
>*>)::ComplexRemove>::TransformExpr(clang::Expr*) 
/home/kimgr/code/llvm-project/out/main/tools/clang/include/clang/AST/StmtNodes.inc:929:1
  #14 0x0adcd51c 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 
>*>)::ComplexRemove>::TransformExprs(clang::Expr* const*, unsigned int, bool, 
llvm::SmallVectorImpl&, bool*) 
/home/kimgr/code/llvm-project/clang/lib/Sema/TreeTransform.h:4047:29
  #15 0x0adae285 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

First sanity check: applying this patch on my branch causes no test failures 
either in the `tools/clang/unittests/Tooling/ToolingTests` or `ninja 
check-clang-semacxx`. Hopefully I can think this through more deeply soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119095

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

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Thanks for the heads-up! I'm a little busy this week, but I need to think about 
potential interference between these two changes. Off the top of my head it 
looks like they should get along fine, but there may be subtleties.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119095

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

[PATCH] D119476: Generalize and harmonize sub-expression traversal

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: clang/lib/AST/Expr.cpp:1903
+// Skip over implicit nodes produced as part of semantic analysis.
+// Designed for use with IgnoreExpreNodes.
+Expr *ignoreImplicitSemaNodes(Expr *E) {

Typo: IgnoreExpr*e*Nodes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119476

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

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a subscriber: aaron.ballman.
kimgr added a comment.

@aaron.ballman This patch replaces https://reviews.llvm.org/D117391 as a 
potential solution for https://github.com/llvm/llvm-project/issues/53044.

I can't swear that the new diagnostics in cxx2a-consteval.cpp are 
standards-conforming, but I think they are no less correct than what's already 
there. Fingers crossed :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119477

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

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

[Kim Gräsman via Phabricator via cfe-commits]

kimgr created this revision.
kimgr requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Full-expressions are Sema-generated implicit nodes that cover
constant-expressions and expressions-with-cleanup for temporaries.

Ignore those as part of implicit-ignore, and also remove too-aggressive
IgnoreImplicit (which includes nested ImplicitCastExprs, for example) on
unpacked sub-expressions.

Add some unittests to demonstrate that RecursiveASTVisitor sees through
ConstantExpr nodes correctly.

Adjust cxx2a-consteval test to cover diagnostics for nested consteval
expressions that were previously missed.

Fixes bug #53044.

Depends on D119476 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119477

Files:
  clang/lib/AST/Expr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp
  clang/unittests/Tooling/CastExprTest.cpp

Index: clang/unittests/Tooling/CastExprTest.cpp
===
--- clang/unittests/Tooling/CastExprTest.cpp
+++ clang/unittests/Tooling/CastExprTest.cpp
@@ -14,12 +14,19 @@
 
 struct CastExprVisitor : TestVisitor {
   std::function OnExplicitCast;
+  std::function OnCast;
 
   bool VisitExplicitCastExpr(ExplicitCastExpr *Expr) {
 if (OnExplicitCast)
   OnExplicitCast(Expr);
 return true;
   }
+
+  bool VisitCastExpr(CastExpr *Expr) {
+if (OnCast)
+  OnCast(Expr);
+return true;
+  }
 };
 
 TEST(CastExprTest, GetSubExprAsWrittenThroughMaterializedTemporary) {
@@ -54,4 +61,57 @@
   CastExprVisitor::Lang_CXX2a);
 }
 
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// constructor conversions (https://github.com/llvm/llvm-project/issues/53044):
+//
+// `-ImplicitCastExpr 'X' 
+//   `-ConstantExpr 'X'
+// |-value: Struct
+// `-CXXConstructExpr 'X' 'void (const char *)'
+//   `-ImplicitCastExpr 'const char *' 
+// `-StringLiteral 'const char [7]' lvalue "foobar"
+TEST(CastExprTest, GetCtorConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+if (Expr->getCastKind() == CK_ConstructorConversion) {
+  auto *Conv = Expr->getConversionFunction();
+  EXPECT_TRUE(isa(Conv))
+  << "Expected CXXConstructorDecl, but saw " << Conv->getDeclKindName();
+}
+  };
+  Visitor.runOver("struct X { consteval X(const char *) {} };\n"
+  "void f() { X x = \"foobar\"; }\n",
+  CastExprVisitor::Lang_CXX2a);
+}
+
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// user-defined conversions.
+//
+// `-ImplicitCastExpr 'const char *' 
+//   `-ConstantExpr 'const char *'
+// |-value: LValue
+// `-CXXMemberCallExpr 'const char *'
+//   `-MemberExpr '' .operator const char *
+// `-DeclRefExpr 'const X' lvalue Var 'x' 'const X'
+TEST(CastExprTest, GetUserDefinedConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+if (Expr->getCastKind() == CK_UserDefinedConversion) {
+  auto *Conv = Expr->getConversionFunction();
+  EXPECT_TRUE(isa(Conv))
+  << "Expected CXXMethodDecl, but saw " << Conv->getDeclKindName();
+}
+  };
+  Visitor.runOver("struct X {\n"
+  "  consteval operator const char *() const {\n"
+  "return nullptr;\n"
+  "  }\n"
+  "};\n"
+  "const char *f() {\n"
+  "  constexpr X x;\n"
+  "  return x;\n"
+  "}\n",
+  CastExprVisitor::Lang_CXX2a);
+}
+
 } // namespace
Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -359,22 +359,29 @@
   // expected-note@-1 {{is not a constant expression}}
   { A k = to_lvalue_ref(A()); } // expected-error {{is not a constant expression}}
   // expected-note@-1 {{is not a constant expression}} expected-note@-1 {{temporary created here}}
-  { A k = to_lvalue_ref(A().ret_a()); } // expected-error {{is not a constant expression}}
-  // expected-note@-1 {{is not a constant expression}} expected-note@-1 {{temporary created here}}
-  { int k = A().ret_a().ret_i(); }
-  { int k = by_value_a(A()); }
+  { A k = to_lvalue_ref(A().ret_a()); } // expected-error {{'alloc::A::ret_a' is not a constant expression}} expected-error {{'alloc::to_lvalue_ref' is not a constant expression}}
+  // expected-note@-1 {{temporary created here}}
+  // expected-note@-2 {{heap-allocated object is not a constant expression}} expected-note@-2 {{reference to temporary is not a constant expression}}
+  { int k = A().ret_a().ret_i(); } // expected-error {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-1 {{heap-allocated object is 

[PATCH] D119476: Generalize and harmonize sub-expression traversal

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a subscriber: aaron.ballman.
kimgr added a comment.

@aaron.ballman First refactor to generalize and harmonize `getSubExprAsWritten` 
and `getConversionFunction`. As mentioned in the commit message, this is 
strictly speaking a functional change, but it should have no visible effect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119476

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

[PATCH] D119476: Generalize and harmonize sub-expression traversal

[Kim Gräsman via Phabricator via cfe-commits]

kimgr created this revision.
Herald added a subscriber: kristof.beyls.
kimgr requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

CastExpr::getSubExprAsWritten and getConversionFunction used to have
disparate implementations to traverse the sub-expression chain and skip
so-called "implicit temporaries" (which are really implicit nodes added by
Sema to represent semantic details in the AST).

There's some friction in these algorithms that makes it hard to extend and
change them:

- skipImplicitTemporary is order-dependent; it can skip a CXXBindTemporaryExpr 
nested inside a MaterializeTemporaryExpr, but not vice versa
- skipImplicitTemporary only runs one pass, it does not traverse multiple 
nested sequences of MTE/CBTE/MTE/CBTE, for example

Both of these weaknesses are void at this point, because this kind of
out-of-order multi-level nesting does not exist in the current AST.

Adding a new implicit expression to skip exacerbates the problem, however,
since a node X might show up in any and all locations between the existing.

Thus;

- Harmonize the form of getSubExprAsWritten and getConversionFunction so they 
both use a for loop
- Use the IgnoreExprNodes machinery to skip multiple nodes
- Rename skipImplicitTemporary to ignoreImplicitSemaNodes to generalize
- Update ignoreImplicitSemaNodes so it only skips one level per call, to mirror 
existing Ignore functions and work better with IgnoreExprNodes

This is a functional change, but one without visible effect.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119476

Files:
  clang/lib/AST/Expr.cpp


Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1899,51 +1899,48 @@
 }
 
 namespace {
-  const Expr *skipImplicitTemporary(const Expr *E) {
-// Skip through reference binding to temporary.
-if (auto *Materialize = dyn_cast(E))
-  E = Materialize->getSubExpr();
+// Skip over implicit nodes produced as part of semantic analysis.
+// Designed for use with IgnoreExpreNodes.
+Expr *ignoreImplicitSemaNodes(Expr *E) {
+  if (auto *Materialize = dyn_cast(E))
+return Materialize->getSubExpr();
 
-// Skip any temporary bindings; they're implicit.
-if (auto *Binder = dyn_cast(E))
-  E = Binder->getSubExpr();
+  if (auto *Binder = dyn_cast(E))
+return Binder->getSubExpr();
 
-return E;
-  }
+  return E;
 }
+} // namespace
 
 Expr *CastExpr::getSubExprAsWritten() {
   const Expr *SubExpr = nullptr;
-  const CastExpr *E = this;
-  do {
-SubExpr = skipImplicitTemporary(E->getSubExpr());
+
+  for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) {
+SubExpr = IgnoreExprNodes(E->getSubExpr(), ignoreImplicitSemaNodes);
 
 // Conversions by constructor and conversion functions have a
 // subexpression describing the call; strip it off.
-if (E->getCastKind() == CK_ConstructorConversion)
-  SubExpr =
-
skipImplicitTemporary(cast(SubExpr->IgnoreImplicit())->getArg(0));
-else if (E->getCastKind() == CK_UserDefinedConversion) {
+if (E->getCastKind() == CK_ConstructorConversion) {
+  SubExpr = IgnoreExprNodes(
+  cast(SubExpr->IgnoreImplicit())->getArg(0),
+  ignoreImplicitSemaNodes);
+} else if (E->getCastKind() == CK_UserDefinedConversion) {
   SubExpr = SubExpr->IgnoreImplicit();
-  assert((isa(SubExpr) ||
-  isa(SubExpr)) &&
+  assert((isa(SubExpr) || isa(SubExpr)) &&
  "Unexpected SubExpr for CK_UserDefinedConversion.");
   if (auto *MCE = dyn_cast(SubExpr))
 SubExpr = MCE->getImplicitObjectArgument();
 }
+  }
 
-// If the subexpression we're left with is an implicit cast, look
-// through that, too.
-  } while ((E = dyn_cast(SubExpr)));
-
-  return const_cast(SubExpr);
+  return const_cast(SubExpr);
 }
 
 NamedDecl *CastExpr::getConversionFunction() const {
   const Expr *SubExpr = nullptr;
 
   for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) {
-SubExpr = skipImplicitTemporary(E->getSubExpr());
+SubExpr = IgnoreExprNodes(E->getSubExpr(), ignoreImplicitSemaNodes);
 
 if (E->getCastKind() == CK_ConstructorConversion)
   return cast(SubExpr)->getConstructor();


Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1899,51 +1899,48 @@
 }
 
 namespace {
-  const Expr *skipImplicitTemporary(const Expr *E) {
-// Skip through reference binding to temporary.
-if (auto *Materialize = dyn_cast(E))
-  E = Materialize->getSubExpr();
+// Skip over implicit nodes produced as part of semantic analysis.
+// Designed for use with IgnoreExpreNodes.
+Expr *ignoreImplicitSemaNodes(Expr *E) {
+  if (auto *Materialize = dyn_cast(E))
+return Materialize->getSubExpr();
 
-// Skip any temporary bindings; they're implicit.

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@aaron.ballman Thanks for the valuable feedback!

> Assuming that the `to_lvalue_ref(A{})` case should diagnose, then yes, I 
> agree, I think the `ret_a()` portion should as well.

My primary focus is not to make sure Clang follows the standard closely for 
`consteval`, but rather to make sure it's self-consistent and doesn't provoke 
undefined behavior for `consteval` inputs.

So another way to think about it is that `to_lvalue_ref` and `ret_a` might both 
diagnose, or not, but absence of diagnostics should not be a result of 
misinterpreting the AST the way we do now.

I'm preparing a different patch to do the short-circuiting in 
`skipImplicitTemporary`, so we can more easily compare and contrast.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I have now convinced myself that including `FullExpr` in 
`skipImplicitTemporary` gives an improvement in `consteval` diagnostics. But 
I'm still not sure why. Motivating example, derived from cxx2a-consteval.cpp:

  struct A {
int *p = new int(42);
consteval A ret_a() const {
return A{};
}
  };
  
  consteval const A _lvalue_ref(const A &) {
return a;
  }
  
  void test() {
constexpr A a{nullptr};
  
// error: call to consteval function 'A::ret_a' is not a constant expression
// { (void)a.ret_a(); }
  
// error: call to consteval function 'to_lvalue_ref' is not a constant 
expression
// { (void)to_lvalue_ref(A{}); }
  
// error: call to consteval function 'to_lvalue_ref' is not a constant 
expression
// but should probably also raise
// error: call to consteval function 'A::ret_a' is not a constant expression
{ (void)to_lvalue_ref(a.ret_a()); }
  }

It's interesting to experiment with these one by one

- `A::ret_a` returns a new A, whose constructor does heap allocation; no 
consteval possible
- `to_lvalue_ref` attempts to return a reference to a temporary; no consteval 
possible

Composing the two as in the last example, it seems to me, should print both 
diagnostics. Mainline Clang doesn't, but changing `skipImplicitTemporary` to 
also skip `FullExpr`s does (also allowing removal of all `IgnoreImplicit` from 
`getSubExprAsWritten` and `getConversionFunction`).

It seems intuitively right to me. I'm just a little peeved that I can't figure 
out the connection between the diagnostic emission and `skipImplicitTemporary`.




Comment at: clang/lib/AST/Expr.cpp:1946-1947
   for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) {
 SubExpr = skipImplicitTemporary(E->getSubExpr());
+SubExpr = SubExpr->IgnoreImplicit();
 

davrec wrote:
> aaron.ballman wrote:
> > `IgnoreImplicit()` calls `IgnoreImplicitSingleStep()` eventually, and that 
> > call does the same work as `skipImplicitTemporary()`, so I think the end 
> > result here should be the same.
> As I look at this a second time, I just realized...calling IgnoreImplicit 
> here mean that the loop only ever runs one iteration, since IgnoreImplicit 
> presumably skips over ImplicitCastExprs.  While I liked how Kim did revision 
> initially because it seemed to handle the constructor conversions similarly 
> to their handling of getSubExprAsWritten() above, now I think something 
> different is needed here.
> 
> Proposed alternative:
> Right now skipIimplicitTemporary does what IgnoreImplicit does *except* skip 
> over a) ImplicitCastExprs and b) FullExprs (= ConstantExprs or 
> ExprWithCleanups).
> 
> Kim has identified that we need to skip over at least ConstantExprs at least 
> in this case (i.e. the most conservative possible fix would be to skip over 
> ConstantExprs just before the cast in line 1950).
> 
> But perhaps the better solution, to forestall future bugs, is to skip over 
> FullExprs in skipImplicitTemporary, so that it skips over everything 
> IgnoreImplicit does except ImplicitCastExprs.  (And, some documentation 
> should be added to `skipImplicitTemporary` to that effect, to aid future 
> maintenance.)
> 
> I can't see offhand how the other uses of skipImplicitTemporary would be 
> negatively affected by additionally skipping over FullExprs.
> 
> Aaron what do you think?  Kim can you verify this alternative would also 
> solve the problem without breaking any tests?
> 
@aaron.ballman Just removing the `skipImplicitTemporary` line is probably not 
going to work, since that's the first time `SubExpr` is assigned a 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: clang/lib/AST/Expr.cpp:1949-1950
 
 if (E->getCastKind() == CK_ConstructorConversion)
   return cast(SubExpr)->getConstructor();
 

kimgr wrote:
> davrec wrote:
> > I think the errors prove we should fall back to the most conservative 
> > possible fix: remove all the other changes and just change these 2 lines to:
> > ```
> > if (E->getCastKind() == CK_ConstructorConversion) {
> >   if (auto *CE = dyn_cast(SubExpr)
> > SubExpr = CE->getSubExpr();
> >   return cast(SubExpr)->getConstructor();
> > }
> > ```
> > I'm can't quite remember but I believe that would solve the bug as narrowly 
> > as possible.  @kimgr can you give it a try if and see if it avoids the 
> > errors?
> > (If it doesn't, I'm stumped...)
> Yes, it does. I needed to add the same `ConstantExpr` skipping to the 
> user-defined conversion handling below, but once those two were in place the 
> new unittests succeed and the existing lit tests also.
> 
> It does feel a little awkward, though... I wish I had a clearer mental model 
> of how the implicit-skipping is supposed to work. My intuition is telling me 
> `skipImplicitTemporary` should probably disappear and we should use the 
> `IgnoreExpr.h` facilities directly instead, but it's all a little fuzzy to me 
> at this point.
> 
> I'll run the full `check-clang` suite overnight with this alternative patch, 
> I have no reason to think there will be problems, but I'll make sure in the 
> morning.
> 
> Thanks!
I can confirm that full `check-clang` also works with the narrower fix. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: clang/lib/AST/Expr.cpp:1949-1950
 
 if (E->getCastKind() == CK_ConstructorConversion)
   return cast(SubExpr)->getConstructor();
 

davrec wrote:
> I think the errors prove we should fall back to the most conservative 
> possible fix: remove all the other changes and just change these 2 lines to:
> ```
> if (E->getCastKind() == CK_ConstructorConversion) {
>   if (auto *CE = dyn_cast(SubExpr)
> SubExpr = CE->getSubExpr();
>   return cast(SubExpr)->getConstructor();
> }
> ```
> I'm can't quite remember but I believe that would solve the bug as narrowly 
> as possible.  @kimgr can you give it a try if and see if it avoids the errors?
> (If it doesn't, I'm stumped...)
Yes, it does. I needed to add the same `ConstantExpr` skipping to the 
user-defined conversion handling below, but once those two were in place the 
new unittests succeed and the existing lit tests also.

It does feel a little awkward, though... I wish I had a clearer mental model of 
how the implicit-skipping is supposed to work. My intuition is telling me 
`skipImplicitTemporary` should probably disappear and we should use the 
`IgnoreExpr.h` facilities directly instead, but it's all a little fuzzy to me 
at this point.

I'll run the full `check-clang` suite overnight with this alternative patch, I 
have no reason to think there will be problems, but I'll make sure in the 
morning.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Here's the diff between my patch (main) and handling `FullExpr` in 
`skipImplicitTemporary` as in the diff I posted above (patch.txt):

  --- main.txt  2022-02-02 20:37:21.619534225 +0100
  +++ patch.txt 2022-02-02 20:34:17.016949227 +0100
  @@ -192,6 +192,13 @@
   /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:360:25: 
note: temporary created here
 { A k = to_lvalue_ref(A()); } // expected-error {{is not a constant 
expression}}
   ^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:362:25: 
error: call to consteval function 'alloc::A::ret_a' is not a constant expression
  +  { A k = to_lvalue_ref(A().ret_a()); } // expected-error {{is not a 
constant expression}}
  +^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:362:25: 
note: pointer to heap-allocated object is not a constant expression
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:334:12: 
note: heap allocation performed here
  +  int* p = new int(42);
  +   ^
   /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:362:11: 
error: call to consteval function 'alloc::to_lvalue_ref' is not a constant 
expression
 { A k = to_lvalue_ref(A().ret_a()); } // expected-error {{is not a 
constant expression}}
 ^
  @@ -199,6 +206,27 @@
   /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:362:25: 
note: temporary created here
 { A k = to_lvalue_ref(A().ret_a()); } // expected-error {{is not a 
constant expression}}
   ^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:364:13: 
error: call to consteval function 'alloc::A::ret_a' is not a constant expression
  +  { int k = A().ret_a().ret_i(); }
  +^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:364:13: 
note: pointer to heap-allocated object is not a constant expression
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:334:12: 
note: heap allocation performed here
  +  int* p = new int(42);
  +   ^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:370:25: 
error: call to consteval function 'alloc::A::ret_a' is not a constant expression
  +  { int k = const_a_ref(A().ret_a()); }
  +^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:370:25: 
note: pointer to heap-allocated object is not a constant expression
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:334:12: 
note: heap allocation performed here
  +  int* p = new int(42);
  +   ^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:371:39: 
error: call to consteval function 'alloc::A::ret_a' is not a constant expression
  +  { int k = const_a_ref(to_lvalue_ref(A().ret_a())); }
  +  ^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:371:39: 
note: pointer to heap-allocated object is not a constant expression
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:334:12: 
note: heap allocation performed here
  +  int* p = new int(42);
  +   ^
   /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:375:14: 
error: call to consteval function 'alloc::A::ret_a' is not a constant expression
 { int k = (A().ret_a(), A().ret_i()); }// expected-error {{is not a 
constant expression}}
^
  @@ -206,6 +234,13 @@
   /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:334:12: 
note: heap allocation performed here
 int* p = new int(42);
  ^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:377:26: 
error: call to consteval function 'alloc::A::ret_a' is not a constant expression
  +  { int k = (const_a_ref(A().ret_a()), A().ret_i()); }
  + ^
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:377:26: 
note: pointer to heap-allocated object is not a constant expression
  +/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:334:12: 
note: heap allocation performed here
  +  int* p = new int(42);
  +   ^
   /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:400:7: 
error: call to consteval function 'self_referencing::f' is not a constant 
expression
 s = f(0); // expected-error {{is not a constant expression}}
 ^
  @@ -454,9 +489,9 @@
   /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:548:15: 
note: declared here
   consteval int f_eval() { // expected-note+ {{declared here}}
 ^
  -/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp:574:25: 
error: cannot take address of consteval function 'f_eval' outside of an 
immediate invocation
  -  { Copy c = Copy(Copy(_eval)); }// expected-error {{cannot 

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Alright, with this diff:

  diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
  index d502b3f1e93e..1716ac0be7ef 100644
  --- a/clang/lib/AST/Expr.cpp
  +++ b/clang/lib/AST/Expr.cpp
  @@ -1908,6 +1908,9 @@ namespace {
   if (auto *Binder = dyn_cast(E))
 E = Binder->getSubExpr();
   
  +if (auto *Full = dyn_cast(E))
  +  E = Full->getSubExpr();
  +
   return E;
 }
   }
  @@ -1944,7 +1947,6 @@ NamedDecl *CastExpr::getConversionFunction() const {
   
 for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) 
{
   SubExpr = skipImplicitTemporary(E->getSubExpr());
  -SubExpr = SubExpr->IgnoreImplicit();
   
   if (E->getCastKind() == CK_ConstructorConversion)
 return cast(SubExpr)->getConstructor();

I get these test failures for `ninja check-clang`:

  FAIL: Clang :: SemaCXX/cxx2a-consteval.cpp (71 of 970)
   TEST 'Clang :: SemaCXX/cxx2a-consteval.cpp' FAILED 

  Script:
  --
  : 'RUN: at line 1';   /home/kimgr/code/llvm-project/out/main/bin/clang -cc1 
-internal-isystem 
/home/kimgr/code/llvm-project/out/main/lib/clang/15.0.0/include -nostdsysteminc 
-std=c++2a -emit-llvm-only -Wno-unused-value 
/home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp -verify
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  error: 'error' diagnostics seen but not expected: 
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 362: call to consteval function 'alloc::to_lvalue_ref' is not a constant 
expression
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 364: call to consteval function 'alloc::A::ret_a' is not a constant 
expression
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 370: call to consteval function 'alloc::A::ret_a' is not a constant 
expression
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 371: call to consteval function 'alloc::A::ret_a' is not a constant 
expression
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 377: call to consteval function 'alloc::A::ret_a' is not a constant 
expression
  error: 'note' diagnostics seen but not expected: 
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 362: reference to temporary is not a constant expression
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 364: pointer to heap-allocated object is not a constant expression
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 370: pointer to heap-allocated object is not a constant expression
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 371: pointer to heap-allocated object is not a constant expression
File /home/kimgr/code/llvm-project/clang/test/SemaCXX/cxx2a-consteval.cpp 
Line 377: pointer to heap-allocated object is not a constant expression
  10 errors generated.

I'm going to dive in and see if I can judge who is right, tests or code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@aaron.ballman @davrec Thank you both!

I started with @davrec's suggestion from 
https://github.com/llvm/llvm-project/issues/53044#issuecomment-1006894536, and 
while it fixed the problem it broke several other consteval lit tests.

It could be that those test breaks are benign, but they startled me a little, 
so I decided to narrow the fix.

I'll try and rebase on latest and experiment with a few different approaches 
and report back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@aaron.ballman Thanks! Please use Kim Gräsman and kim.grasman at gmail.com.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117390

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@rsmith Gentle ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@rsmith Gentle ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117390

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

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

We've had reports from users that it's now possible to build IWYU together with 
Clang and LLVM using LLVM_EXTERNAL_PROJECTS, so this can be closed.


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

https://reviews.llvm.org/D31696

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@rsmith Weekly ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@rsmith Weekly ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117390

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

- Update broken test case
- Rebase on latest main (8eb74626f 
)
- Build and run `ninja check-clang`

I don't have commit access, so I would appreciate help landing. Let me know if 
there's anything else I can do before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr updated this revision to Diff 400381.
kimgr added a comment.

Fix spurious semicolon


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

Files:
  clang/lib/AST/Expr.cpp
  clang/unittests/Tooling/CastExprTest.cpp


Index: clang/unittests/Tooling/CastExprTest.cpp
===
--- clang/unittests/Tooling/CastExprTest.cpp
+++ clang/unittests/Tooling/CastExprTest.cpp
@@ -14,12 +14,19 @@
 
 struct CastExprVisitor : TestVisitor {
   std::function OnExplicitCast;
+  std::function OnCast;
 
   bool VisitExplicitCastExpr(ExplicitCastExpr *Expr) {
 if (OnExplicitCast)
   OnExplicitCast(Expr);
 return true;
   }
+
+  bool VisitCastExpr(CastExpr *Expr) {
+if (OnCast)
+  OnCast(Expr);
+return true;
+  }
 };
 
 TEST(CastExprTest, GetSubExprAsWrittenThroughMaterializedTemporary) {
@@ -54,4 +61,57 @@
   CastExprVisitor::Lang_CXX2a);
 }
 
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// constructor conversions (https://github.com/llvm/llvm-project/issues/53044):
+//
+// `-ImplicitCastExpr 'S' 
+// `-ConstantExpr 'S'
+//   |-value: Struct
+//   `-CXXConstructExpr 'S' 'void (const char *)'
+// `-ImplicitCastExpr 'const char *' 
+//   `-StringLiteral 'const char [7]' lvalue "foobar"
+TEST(CastExprTest, GetCtorConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+if (Expr->getCastKind() == CK_ConstructorConversion) {
+  auto *Conv = Expr->getConversionFunction();
+  EXPECT_TRUE(isa(Conv))
+  << "Expected CXXConstructorDecl, but saw " << 
Conv->getDeclKindName();
+}
+  };
+  Visitor.runOver("struct X { consteval X(const char *) {} };\n"
+  "void f() { X x = \"foobar\"; }\n",
+  CastExprVisitor::Lang_CXX2a);
+}
+
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// user-defined conversions.
+//
+// `-ImplicitCastExpr 'const char *' 
+//   `-ConstantExpr 'const char *'
+// |-value: LValue
+// `-CXXMemberCallExpr 'const char *'
+//   `-MemberExpr '' .operator const char *
+// `-DeclRefExpr 'const X' lvalue Var 'x' 'const X'
+TEST(CastExprTest, GetUserDefinedConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+if (Expr->getCastKind() == CK_UserDefinedConversion) {
+  auto *Conv = Expr->getConversionFunction();
+  EXPECT_TRUE(isa(Conv))
+  << "Expected CXXMethodDecl, but saw " << Conv->getDeclKindName();
+}
+  };
+  Visitor.runOver("struct X {\n"
+  "  consteval operator const char *() const {\n"
+  "return nullptr;\n"
+  "  }\n"
+  "};\n"
+  "const char *f() {\n"
+  "  constexpr X x;\n"
+  "  return x;\n"
+  "}\n",
+  CastExprVisitor::Lang_CXX2a);
+}
+
 } // namespace
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1944,6 +1944,7 @@
 
   for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) {
 SubExpr = skipImplicitTemporary(E->getSubExpr());
+SubExpr = SubExpr->IgnoreImplicit();
 
 if (E->getCastKind() == CK_ConstructorConversion)
   return cast(SubExpr)->getConstructor();


Index: clang/unittests/Tooling/CastExprTest.cpp
===
--- clang/unittests/Tooling/CastExprTest.cpp
+++ clang/unittests/Tooling/CastExprTest.cpp
@@ -14,12 +14,19 @@
 
 struct CastExprVisitor : TestVisitor {
   std::function OnExplicitCast;
+  std::function OnCast;
 
   bool VisitExplicitCastExpr(ExplicitCastExpr *Expr) {
 if (OnExplicitCast)
   OnExplicitCast(Expr);
 return true;
   }
+
+  bool VisitCastExpr(CastExpr *Expr) {
+if (OnCast)
+  OnCast(Expr);
+return true;
+  }
 };
 
 TEST(CastExprTest, GetSubExprAsWrittenThroughMaterializedTemporary) {
@@ -54,4 +61,57 @@
   CastExprVisitor::Lang_CXX2a);
 }
 
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// constructor conversions (https://github.com/llvm/llvm-project/issues/53044):
+//
+// `-ImplicitCastExpr 'S' 
+// `-ConstantExpr 'S'
+//   |-value: Struct
+//   `-CXXConstructExpr 'S' 'void (const char *)'
+// `-ImplicitCastExpr 'const char *' 
+//   `-StringLiteral 'const char [7]' lvalue "foobar"
+TEST(CastExprTest, GetCtorConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+if (Expr->getCastKind() == CK_ConstructorConversion) {
+  auto *Conv = Expr->getConversionFunction();
+  EXPECT_TRUE(isa(Conv))
+  << "Expected CXXConstructorDecl, but saw " << 

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: clang/unittests/Tooling/CastExprTest.cpp:111
+  "const char *f() {\n"
+  "  constexpr X x;\n";
+  "  return x;\n"

Oops, spurious semicolon here. Will post a new version once I've tested it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@daverec I don't have commit access, could you help me land this?

I've rebased on `main` without conflicts, and `ninja check-clang` ran 
successfully after rebase.

Let me know if there's anything else I can do as far as prep work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117390

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Oh yeah, this closes bug https://github.com/llvm/llvm-project/issues/53044. Do 
I mention that in the commit message/patch summary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr created this revision.
kimgr requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Under -std=c++20 with use of consteval, the subexpression hierarchy sometimes
contains ConstantExpr nodes that getConversionFunction did not expect.

CastExpr::getConversionFunction is very rarely used, so this was difficult to
trigger in the compiler. It is possible to reproduce using -ast-dump=json, which
triggers an assertion for inputs with consteval implicit casts.

In AST-based tools, however, it surfaces quite easily if they try to inspect the
conversion function of a visited CastExpr.

Add two new testcases, both for implicit constructor conversions and
user-defined conversions with consteval.

Depends on D117390 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117391

Files:
  clang/lib/AST/Expr.cpp
  clang/unittests/Tooling/CastExprTest.cpp


Index: clang/unittests/Tooling/CastExprTest.cpp
===
--- clang/unittests/Tooling/CastExprTest.cpp
+++ clang/unittests/Tooling/CastExprTest.cpp
@@ -14,12 +14,19 @@
 
 struct CastExprVisitor : TestVisitor {
   std::function OnExplicitCast;
+  std::function OnCast;
 
   bool VisitExplicitCastExpr(ExplicitCastExpr *Expr) {
 if (OnExplicitCast)
   OnExplicitCast(Expr);
 return true;
   }
+
+  bool VisitCastExpr(CastExpr *Expr) {
+if (OnCast)
+  OnCast(Expr);
+return true;
+  }
 };
 
 TEST(CastExprTest, GetSubExprAsWrittenThroughMaterializedTemporary) {
@@ -54,4 +61,57 @@
   CastExprVisitor::Lang_CXX2a);
 }
 
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// constructor conversions (https://github.com/llvm/llvm-project/issues/53044):
+//
+// `-ImplicitCastExpr 'S' 
+// `-ConstantExpr 'S'
+//   |-value: Struct
+//   `-CXXConstructExpr 'S' 'void (const char *)'
+// `-ImplicitCastExpr 'const char *' 
+//   `-StringLiteral 'const char [7]' lvalue "foobar"
+TEST(CastExprTest, GetCtorConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+if (Expr->getCastKind() == CK_ConstructorConversion) {
+  auto *Conv = Expr->getConversionFunction();
+  EXPECT_TRUE(isa(Conv))
+  << "Expected CXXConstructorDecl, but saw " << 
Conv->getDeclKindName();
+}
+  };
+  Visitor.runOver("struct X { consteval X(const char *) {} };\n"
+  "void f() { X x = \"foobar\"; }\n",
+  CastExprVisitor::Lang_CXX2a);
+}
+
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// user-defined conversions.
+//
+// `-ImplicitCastExpr 'const char *' 
+//   `-ConstantExpr 'const char *'
+// |-value: LValue
+// `-CXXMemberCallExpr 'const char *'
+//   `-MemberExpr '' .operator const char *
+// `-DeclRefExpr 'const X' lvalue Var 'x' 'const X'
+TEST(CastExprTest, GetUserDefinedConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+if (Expr->getCastKind() == CK_UserDefinedConversion) {
+  auto *Conv = Expr->getConversionFunction();
+  EXPECT_TRUE(isa(Conv))
+  << "Expected CXXMethodDecl, but saw " << Conv->getDeclKindName();
+}
+  };
+  Visitor.runOver("struct X {\n"
+  "  consteval operator const char *() const {\n"
+  "return nullptr;\n"
+  "  }\n"
+  "};\n"
+  "const char *f() {\n"
+  "  constexpr X x;\n";
+  "  return x;\n"
+  "}\n",
+  CastExprVisitor::Lang_CXX2a);
+}
+
 } // namespace
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1944,6 +1944,7 @@
 
   for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) {
 SubExpr = skipImplicitTemporary(E->getSubExpr());
+SubExpr = SubExpr->IgnoreImplicit();
 
 if (E->getCastKind() == CK_ConstructorConversion)
   return cast(SubExpr)->getConstructor();


Index: clang/unittests/Tooling/CastExprTest.cpp
===
--- clang/unittests/Tooling/CastExprTest.cpp
+++ clang/unittests/Tooling/CastExprTest.cpp
@@ -14,12 +14,19 @@
 
 struct CastExprVisitor : TestVisitor {
   std::function OnExplicitCast;
+  std::function OnCast;
 
   bool VisitExplicitCastExpr(ExplicitCastExpr *Expr) {
 if (OnExplicitCast)
   OnExplicitCast(Expr);
 return true;
   }
+
+  bool VisitCastExpr(CastExpr *Expr) {
+if (OnCast)
+  OnCast(Expr);
+return true;
+  }
 };
 
 TEST(CastExprTest, GetSubExprAsWrittenThroughMaterializedTemporary) {
@@ -54,4 +61,57 @@
   CastExprVisitor::Lang_CXX2a);
 }
 
+// Verify that getConversionFunction looks through a ConstantExpr for implicit

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

[Kim Gräsman via Phabricator via cfe-commits]

kimgr created this revision.
kimgr requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In preparation for adding new tests. No functional change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117390

Files:
  clang/unittests/Tooling/CastExprTest.cpp


Index: clang/unittests/Tooling/CastExprTest.cpp
===
--- clang/unittests/Tooling/CastExprTest.cpp
+++ clang/unittests/Tooling/CastExprTest.cpp
@@ -23,15 +23,15 @@
 };
 
 TEST(CastExprTest, GetSubExprAsWrittenThroughMaterializedTemporary) {
-CastExprVisitor Visitor;
-Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
-  auto Sub = Expr->getSubExprAsWritten();
-  EXPECT_TRUE(isa(Sub))
+  CastExprVisitor Visitor;
+  Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
+auto Sub = Expr->getSubExprAsWritten();
+EXPECT_TRUE(isa(Sub))
 << "Expected DeclRefExpr, but saw " << Sub->getStmtClassName();
-};
-Visitor.runOver("struct S1 {};\n"
-"struct S2 { operator S1(); };\n"
-"S1 f(S2 s) { return static_cast(s); }\n");
+  };
+  Visitor.runOver("struct S1 {};\n"
+  "struct S2 { operator S1(); };\n"
+  "S1 f(S2 s) { return static_cast(s); }\n");
 }
 
 // Verify that getSubExprAsWritten looks through a ConstantExpr in a scenario
@@ -43,15 +43,15 @@
 // `-CXXConstructExpr 'S' 'void (int)'
 //   `-IntegerLiteral 'int' 0
 TEST(CastExprTest, GetSubExprAsWrittenThroughConstantExpr) {
-CastExprVisitor Visitor;
-Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
-  auto *Sub = Expr->getSubExprAsWritten();
-  EXPECT_TRUE(isa(Sub))
+  CastExprVisitor Visitor;
+  Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
+auto *Sub = Expr->getSubExprAsWritten();
+EXPECT_TRUE(isa(Sub))
 << "Expected IntegerLiteral, but saw " << Sub->getStmtClassName();
-};
-Visitor.runOver("struct S { consteval S(int) {} };\n"
-"S f() { return S(0); }\n",
-CastExprVisitor::Lang_CXX2a);
+  };
+  Visitor.runOver("struct S { consteval S(int) {} };\n"
+  "S f() { return S(0); }\n",
+  CastExprVisitor::Lang_CXX2a);
 }
 
-}
+} // namespace


Index: clang/unittests/Tooling/CastExprTest.cpp
===
--- clang/unittests/Tooling/CastExprTest.cpp
+++ clang/unittests/Tooling/CastExprTest.cpp
@@ -23,15 +23,15 @@
 };
 
 TEST(CastExprTest, GetSubExprAsWrittenThroughMaterializedTemporary) {
-CastExprVisitor Visitor;
-Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
-  auto Sub = Expr->getSubExprAsWritten();
-  EXPECT_TRUE(isa(Sub))
+  CastExprVisitor Visitor;
+  Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
+auto Sub = Expr->getSubExprAsWritten();
+EXPECT_TRUE(isa(Sub))
 << "Expected DeclRefExpr, but saw " << Sub->getStmtClassName();
-};
-Visitor.runOver("struct S1 {};\n"
-"struct S2 { operator S1(); };\n"
-"S1 f(S2 s) { return static_cast(s); }\n");
+  };
+  Visitor.runOver("struct S1 {};\n"
+  "struct S2 { operator S1(); };\n"
+  "S1 f(S2 s) { return static_cast(s); }\n");
 }
 
 // Verify that getSubExprAsWritten looks through a ConstantExpr in a scenario
@@ -43,15 +43,15 @@
 // `-CXXConstructExpr 'S' 'void (int)'
 //   `-IntegerLiteral 'int' 0
 TEST(CastExprTest, GetSubExprAsWrittenThroughConstantExpr) {
-CastExprVisitor Visitor;
-Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
-  auto *Sub = Expr->getSubExprAsWritten();
-  EXPECT_TRUE(isa(Sub))
+  CastExprVisitor Visitor;
+  Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
+auto *Sub = Expr->getSubExprAsWritten();
+EXPECT_TRUE(isa(Sub))
 << "Expected IntegerLiteral, but saw " << Sub->getStmtClassName();
-};
-Visitor.runOver("struct S { consteval S(int) {} };\n"
-"S f() { return S(0); }\n",
-CastExprVisitor::Lang_CXX2a);
+  };
+  Visitor.runOver("struct S { consteval S(int) {} };\n"
+  "S f() { return S(0); }\n",
+  CastExprVisitor::Lang_CXX2a);
 }
 
-}
+} // namespace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@dblaikie I think @sammccall mentioned `IWYU pragma: friend` before, that's 
what I was alluding to. Much of IWYU's complexity comes from maintaining these 
library mappings both statically (using mapping tables) and dynamically (using 
in-source annotations).

I figured the compiler wouldn't want to maintain that level of fidelity for 
mappings, but judging from the subsequent discussion, maybe it could.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106394

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

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Hi, sorry I'm late to the game. IWYU maintainer here.

I saw this patch mentioned in the LLVM Weekly newsletter and immediately 
thought: "wow, great, I have to build support in IWYU for that!".

I too prefer pragmas to magic comments, and I don't think `include_instead` 
necessarily needs to cover all IWYU pragmas 
(https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md).

Took me a while to get my head around it, but I see now that this is only 
supported for system headers. I think that makes sense for the compiler, 
otherwise it will be hard to guess which headers are allowed to include what. 
IWYU usually doesn't have that problem, as we analyze source files 
individually, and usually not headers independently.

My only concern was spelling -- in IWYU we need some handholding to know 
whether to use angled or quoted includes, but I see the quoting is part of the 
pragma, so that should be nice and useful.

Does this support the macro-able `_Pragma` syntax, so that users can be 
portable using something like:

  #ifdef __clang__
  #define INCLUDE_INSTEAD(headername) _Pragma("clang include_instead " ## 
headername)
  #else
  #define INCLUDE_INSTEAD(headername)
  #endif

? That might also make a nice testcase.

I'd love it if you could keep me in the loop if you want to extend this to 
something more general (using something like e.g. `IWYU pragma: friend`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106394

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

[PATCH] D54047: Check TUScope is valid before use

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

This was eventually fixed in IWYU based on @jkorous' suggestion above. I 
believe the problem is/was:

- After code is parsed and the AST is built, Sema resets its `TUScope` member 
to null
- We use Sema to lookup and define default constructors before traversing the 
AST using a RAV
- Some yet-not-fully-identified constructs cause Sema to need a TUScope for 
this (something to do with `LazilyCreateBuiltin`, but I haven't been able to 
construct a reduced testcase)

So in 
https://github.com/include-what-you-use/include-what-you-use/commit/5e7843434169a8af05ebd6e1434cc3cffb66,
 we explicitly re-point `Sema::TUScope` to `Sema::getCurScope()`, which seems 
to have fixed the crash.

This revision can be closed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54047

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

[PATCH] D31697: Check for null before using TUScope

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

This was eventually fixed in IWYU. I believe the problem is/was:

- After code is parsed and the AST is built, Sema resets its `TUScope` member 
to null
- We use Sema to lookup and define default constructors before traversing the 
AST using a RAV
- Some yet-not-fully-identified constructs cause Sema to need a TUScope for 
this (something to do with `LazilyCreateBuiltin`, but I haven't been able to 
construct a reduced testcase)

So in 
https://github.com/include-what-you-use/include-what-you-use/commit/5e7843434169a8af05ebd6e1434cc3cffb66,
 we explicitly re-point `Sema::TUScope` to `Sema::getCurScope()`, which seems 
to have fixed the crash.

This revision can be closed.


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

https://reviews.llvm.org/D31697

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

[PATCH] D72703: Add a warning, flags and pragmas to limit the number of pre-processor tokens in a translation unit

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

In D72703#1834177 , @hans wrote:

> I assume the same correlation could also be found with lines of code, but I 
> think tokens is a better dimension to measure since it's less likely to be 
> gamed, and it also is also kind of the basic work unit that the compiler 
> deals with.


Yeah, and code with lots of documentatinon comments isn't penalized.

>> Could you expose a flag for printing token count so users can run their own 
>> analysis? Or does that already exist in baseline clang? It's easier to set a 
>> maximum for a codebase if the distribution is known.
> 
> I used this patch with -fmax-tokens 1 and scraped the output for my 
> measurements. I would like to avoid adding a separate flag if we can avoid it.

Ah, it didn't occur to me that `-fmax-tokens` itself could be used like this. 
Thanks!

FWIW, I'm not ecstatic about `max_tokens_here`, I thought `max_tokens_lexed` 
had a nicer ring to it. /peanut.


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

https://reviews.llvm.org/D72703



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

[PATCH] D72730: [clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

A small Python suggestion.




Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:112
+
+  if "Notes" in diag:
+for note in diag["Notes"]:

This double lookup is unnecessary, you can do `for note in diag.get("Notes", 
[]):` to default to empty list if "Notes" is not in diag. 



Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:114
+for note in diag["Notes"]:
+  if "Replacements" in note and note["Replacements"]:
+return True

Same here (not sure if Replacements holds a list, but I think it might) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72730



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

[PATCH] D72703: Add a warning, flags and pragmas to limit the number of pre-processor tokens in a translation unit

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I just want to say that finding the correlation between token count and compile 
time is a bit of a breakthrough! Could you expose a flag for printing token 
count so users can run their own analysis? Or does that already exist in 
baseline clang? It's easier to set a maximum for a codebase if the distribution 
is known.

Thanks!


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

https://reviews.llvm.org/D72703



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

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Oh, I see. I thought `rc.exe` was the one and only implementation. Fair enough, 
this is probably the lesser of two evils :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70196



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

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I wonder if it would be better to look at the compile-command than the source 
file?

It's not unthinkable that someone would use another extension for an rc file, 
or use a .rc extension for a C or C++ source file. But if the compiler is 
rc.exe, the source file must be in rc format and should be ignored, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70196



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

[PATCH] D68163: [analyzer][MallocChecker][NFC] Change the use of IdentifierInfo* to CallDescription

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1006
   const FunctionDecl *FD = C.getCalleeDecl(CE);
-  if (!FD)
+  if (!FD || FD->getKind() != Decl::Function)
 return;

NoQ wrote:
> The `FD->getKind() != Decl::Function` part is super mega redundant here.
Sorry for jumping in from nowhere. AFAIK, this is the only way to detect free 
vs member functions. It looks like this wants to discard member functions. Are 
you sure it's redundant? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68163



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

[PATCH] D61909: Add Clang shared library with C++ exports

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

@sylvestre.ledru @beanz  After this change, the Debian packaging on 
apt.llvm.org is basically broken. See 
https://bugs.llvm.org/show_bug.cgi?id=42432. I'm sure this can be fixed in 
packaging, but I don't know enough about it to know exactly what to do. At any 
rate I think the idea that `libclang_shared.so.*` can stay out of the published 
package is wrong. At least unless there's also a way to build the Clang tree 
withouth the `clang_shared` target.

Would be grateful for any ideas on how to fix this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61909



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

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

In D62115#1547705 , @skan wrote:

> In D62115#1547698 , @kimgr wrote:
>
> > This looks good to me, thanks!
>
>
> Could you help accept this patch?


I'm not active in the project, so I don't feel comfortable accepting. See if 
you can find a contributor to sign it off.


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

https://reviews.llvm.org/D62115



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

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

This looks good to me, thanks!


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

https://reviews.llvm.org/D62115



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

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I think the test needs a bit more work. It's essentially checking the same 
thing twice to exercise the Windows path separators.

It looks like there's already a test for `-H` in 
`FrontEnd/print-header-includes.c`. That also demonstrates the use of 
`--check-prefix` to handle target-specific stuff. Maybe you could fold this 
into there?


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

https://reviews.llvm.org/D62115



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

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Also, consider `././Inputs/empty.h`.


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

https://reviews.llvm.org/D62115



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

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I was thinking about a testcase like:

  // RUN: %clang -H -fsyntax-only %s 2>&1 | FileCheck %s
  
  #include "..\Index\Inputs\empty.h"
  #include ".\Inputs\empty.h"
  
  // CHECK: .
  // CHECK-SAME: ???
  // CHECK: .
  // CHECK-NOT: ???
  // CHECK-SAME: ???

with some provision for running it in a Windows environment.

Not sure what the expected behavior should be, hence the `???`


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

https://reviews.llvm.org/D62115



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

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Should you also consider Windows path separators here?


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

https://reviews.llvm.org/D62115



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

[PATCH] D58556: [LibTooling] Add "smart" retrieval of AST-node source to FixIt library

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: clang/include/clang/Tooling/FixIt.h:73
+// context. In contrast with \p getText(), this function selects a source range
+// "automatically", extracting text that a reader might intuitively associate
+// with a node.  Currently, only specialized for \p clang::Stmt, where it will

ymandel wrote:
> kimgr wrote:
> > ymandel wrote:
> > > ilya-biryukov wrote:
> > > > What are other tricky cases you have in mind for the future?
> > > I just assumed that we'd hit more as we dig into them, but, I'm not so 
> > > sure now.  The two others I can think of offhand are 1) extending to 
> > > include associated comments, 2) semicolons after declarations.  Commas 
> > > present a similar challenge (if a bit simpler) when used in a list (vs. 
> > > the comma operator).  Are there any other separators in C++? 
> > > 
> > > At a higher level, it would be nice to align this with your work on tree 
> > > transformations. That is, think of these functions as short-term shims to 
> > > simulate the behavior we'll ultimately get from that new library. 
> > > However, it might be premature to consider those details here.
> > Peanut gallery comment on this:
> > 
> > > The two others I can think of offhand are
> > > 1) extending to include associated comments,
> > > 2) semicolons after declarations.
> > > Commas present a similar challenge (if a bit simpler) when used in a list 
> > > (vs. the comma operator).
> > > Are there any other separators in C++?
> > 
> > Would it make sense to let callers choose what level of expansion they want 
> > with a flag mask? Somehow I think that makes it easier to name the 
> > function, too, e.g.:
> > ```
> > StringRef getExpandedRange(const Stmt , ASTContext , 
> > ExpansionFlags Flags);
> > ```
> > 
> Yes, I think that's a good idea. I even like the name except that it will 
> likely be confused with Expansion locs.  Maybe `getExtendedRange`?
Extended sounds good to me too! 


Repository:
  rC Clang

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

https://reviews.llvm.org/D58556



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

[PATCH] D58556: [LibTooling] Add "smart" retrieval of AST-node source to FixIt library

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: clang/include/clang/Tooling/FixIt.h:73
+// context. In contrast with \p getText(), this function selects a source range
+// "automatically", extracting text that a reader might intuitively associate
+// with a node.  Currently, only specialized for \p clang::Stmt, where it will

ymandel wrote:
> ilya-biryukov wrote:
> > What are other tricky cases you have in mind for the future?
> I just assumed that we'd hit more as we dig into them, but, I'm not so sure 
> now.  The two others I can think of offhand are 1) extending to include 
> associated comments, 2) semicolons after declarations.  Commas present a 
> similar challenge (if a bit simpler) when used in a list (vs. the comma 
> operator).  Are there any other separators in C++? 
> 
> At a higher level, it would be nice to align this with your work on tree 
> transformations. That is, think of these functions as short-term shims to 
> simulate the behavior we'll ultimately get from that new library. However, it 
> might be premature to consider those details here.
Peanut gallery comment on this:

> The two others I can think of offhand are
> 1) extending to include associated comments,
> 2) semicolons after declarations.
> Commas present a similar challenge (if a bit simpler) when used in a list 
> (vs. the comma operator).
> Are there any other separators in C++?

Would it make sense to let callers choose what level of expansion they want 
with a flag mask? Somehow I think that makes it easier to name the function, 
too, e.g.:
```
StringRef getExpandedRange(const Stmt , ASTContext , ExpansionFlags 
Flags);
```



Repository:
  rC Clang

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

https://reviews.llvm.org/D58556



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

[PATCH] D54047: Check TUScope is valid before use

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added subscribers: zturner, kimgr.
kimgr added a comment.

Here's some related suggestions/questions for context:

- Earlier patch from @zturner with a minimal repro case: 
https://reviews.llvm.org/D31697. I don't think this is reproducible with Clang 
proper.
- Open question for alternative solutions: 
https://lists.llvm.org/pipermail/cfe-dev/2018-March/057147.html

FWIW,

- Kim, IWYU maintainer


Repository:
  rC Clang

https://reviews.llvm.org/D54047



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

[PATCH] D49922: [P0936R0] add [[clang::lifetimebound]] attribute

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Potential typo in the tests




Comment at: test/SemaCXX/attr-lifetimebound.cpp:24
+
+  // Do not diagnose non-void return types; they can still be lifetime-bound.
+  long long ptrintcast(int  [[clang::lifetimebound]]) {

Should this say 'non-ref' instead of 'non-void'? 


Repository:
  rC Clang

https://reviews.llvm.org/D49922



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

[PATCH] D49486: [cfe][CMake] Export the clang resource directory

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Thank you for doing this, I'm going to guess you have IWYU in mind :-)

So as consumers of this, how do you envision we use the new variable? Something 
like https://stackoverflow.com/a/13429998 to copy the resource dir into our 
build root, and then an install rule to put it where we want it?


https://reviews.llvm.org/D49486



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

[PATCH] D32696: More detailed docs for UsingShadowDecl

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.
Herald added a subscriber: llvm-commits.

Ping!


Repository:
  rL LLVM

https://reviews.llvm.org/D32696



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

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.
Herald added a reviewer: javed.absar.

This can be closed, IWYU no longer officially supports in-tree builds.


https://reviews.llvm.org/D31696



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

[PATCH] D46652: [clang-cl, PCH] Implement support for MS-style PCH through headers

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.



>> Can you test what happens when you do clang-cl.exe /Yc stdafx.h /Tp 
>> stdafx.h, i.e. compile the header
>  > directly as C++ code and generate .pch from it? The normal MSVC modus 
> operandi is to have stdafx.h
>  > included in all source files (with /Yu) and stdafx.cpp to generate it 
> (with /Yc). That stdafx.cpp file serves
>  > no purpose, so I've played this trick to generate PCH from the header 
> directly. If it works, it might also
>  > be useful for your tests.
> 
> It isn't always the case that the .cpp file serves no purpose. It's true that 
> the MS project generators give you this setup but we've seen many projects 
> over the years that use a regular source file with code in it to generate the 
> PCH. The > object file generated during PCH create can have real code and the 
> compiler can take advantage of that fact by compiling less code in the 
> compilation units that use the PCH [...]

Cool, it's never occurred to me that the source file has any other purpose than 
including the pch header.

> I am not sure about your command: clang-cl.exe /Yc stdafx.h /Tp stdafx.h. The 
> space makes this a compile with just
>  /Yc (no through header). This patch doesn't address /Yc without the through 
> header. Something like clang-cl /Yc 
> /Tpstdafx.h would probably work if/when that is implemented.

I no longer have access to an MSVC environment, so I can't spell out the exact 
command, but I was trying to phrase a command that generates a PCH from the 
through-header *without a distinct source file*. It's proved to be a useful 
trick. I didn't mean to omit the through header, let's try again:

  cl.exe /Ycstdafx.h /TP stdafx.h

or if that doesn't work, a /FI switch may be required, I can't remember for 
sure what we did:

  cl.exe /Ycstdafx.h /TP /FI stdafx.h stdafx.h

>> I think the current skip-warning is over-specific -- what if I put an inline 
>> function before my PCH include?
>  >  A global variable? A #pragma? Or am I misunderstanding what skipping 
> does? It seems to me that any
>  > non-comment tokens before the PCH #include should raise a warning.
> 
> Not any tokens. The through header "feature" compiles all the code in the 
> file up to the through header.
>  So normally you are used to seeing just comments and #include . 
> That's one usage but your
>  PCH can be created from a source like this [...]

Thanks for the example, I must have misunderstood how MSVC PCHs work. With that 
background, the warning seems fine.

>> I find the "through header" terminology is a little hard to interpret, but I 
>> did find it on MSDN, so maybe it's well established.
> 
> Sorry about the terminology. I've been responsible for emulating this PCH 
> mechanism for many years so I may be one
>  of the few that understands it. I did try to keep that terminology out of 
> the diagnostics but I don't really have a better
>  name for through header.

:) Sounds like it's well-established!


https://reviews.llvm.org/D46652



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

[PATCH] D46652: [clang-cl, PCH] Implement support for MS-style PCH through headers

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I've done some terrible things with MSVC PCHs over the years, so I'll pile on 
some more questions:

- Can you test what happens when you do `clang-cl.exe /Yc stdafx.h /Tp 
stdafx.h`, i.e. compile the header directly as C++ code and generate .pch from 
it?  The normal MSVC modus operandi is to have stdafx.h included in all source 
files (with /Yu) and stdafx.cpp to generate it (with /Yc). That stdafx.cpp file 
serves no purpose, so I've played this trick to generate PCH from the header 
directly. If it works, it might also be useful for your tests.
- I think the current skip-warning is over-specific -- what if I put an inline 
function before my PCH include? A global variable? A #pragma? Or am I 
misunderstanding what skipping does? It seems to me that any non-comment tokens 
before the PCH #include should raise a warning.
- I find the "through header" terminology is a little hard to interpret, but I 
did find it on MSDN, so maybe it's well established.

I'll try to look more at this patch in context later.


https://reviews.llvm.org/D46652



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

[PATCH] D46522: [clang] cmake: resolve symlinks in ClangConfig.cmake

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

It works! Having to specify `CMAKE_PREFIX_PATH` is at least documentable, so I 
don't see that as a major drawback, especially if it makes docs for tools using 
Clang more portable.

Thank you for moving this along, now I can vastly simplify our CMake build.


Repository:
  rC Clang

https://reviews.llvm.org/D46522



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

[PATCH] D46522: [clang] cmake: resolve symlinks in ClangConfig.cmake

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I'm interested in this, I've tried for a while to fix the Debian packaging but 
I'm completely new to the packaging toolchain, so I'm making slow headway.

Some of the problems are recorded in: 
https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=862328

My (possibly naive) take is that since the LLVM/Clang build/install tree works 
as-is with `find_package`, the bug must be in packaging. That is, if you have a 
local build tree in `/build/`, this configures without a hitch: `cmake 
-DCMAKE_PREFIX_PATH=/build/ -G Ninja .`.


Repository:
  rC Clang

https://reviews.llvm.org/D46522



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

[PATCH] D46652: [clang-cl, PCH] Implement support for MS-style PCH through headers

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

> When using a PCH, tokens are skipped until after the through header is seen.

I know this is what MSVC does (though in their case I think it's an artifact of 
their implementation), but I remember this being a source of mysterious bugs:

  #define MY_SYMBOL 1
  #include "stdafx.h"  // include pch

The `#define` would silently disappear, and the symbol would never be defined. 
I wonder if it would be worth adding a warning for this in 
`SkipTokensUntilPCHThroughHeader`?


Repository:
  rC Clang

https://reviews.llvm.org/D46652



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

[PATCH] D43578: -ftime-report switch support in Clang

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

In https://reviews.llvm.org/D43578#1067974, @rsmith wrote:

> In https://reviews.llvm.org/D43578#1067891, @kimgr wrote:
>
> > I disagreed up until the last paragraph :)
>
>
> Can you say which things you disagree with? There seem to be two important 
> facts here:
>
> 1. LLVM's timing infrastructure cannot correctly report timings when one 
> function containing a timing region calls another. That will happen all the 
> time with this patch applied.
> 2. Clang's architecture means that if you time the amount of time spent in, 
> say, the "ParseTemplate" function, you have not computed the amount of time 
> spent parsing templates, because the parser calls into Sema, which might 
> perform tasks that are only somewhat related to parsing the template (such as 
> performing other instantiations), and likewise Sema calls into other layers 
> (such as AST file deserialization and code generation).
>
>   The first might have been fixed in LLVM at this point, but I can't see any 
> evidence of that in LLVM's Timer implementation. And the second seems even 
> more fundamental. But if there's some way around that, which would allow us 
> to produce (correct!) numbers for times spent parsing / whatever else, 
> without, for instance, adding a correctness requirement that we annotate 
> every Parser -> Sema entry point with a timer, then that's definitely 
> something we should discuss.


Ah, I see. I read your original message as dismissive as in users don't need to 
know where time is spent.

I tried to make the case that users do care where time is spent, because it can 
give a hint as to what part of their physical/logical design they can work on 
to get better compile times.

But I think you were really saying that LLVM and Clang as designed can't 
provide anything perfect, only slightly misleading information?

Sorry for the misunderstanding.

- Kim


https://reviews.llvm.org/D43578



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

[PATCH] D43578: -ftime-report switch support in Clang

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Heh, my e-mail response was eaten along the way. Continued here:

On Sat, Apr 14, 2018 at 1:53 AM, Richard Smith - zygoloid via
Phabricator via cfe-commits  wrote:

> rsmith added a comment.
> 
> So I don't think this patch is reasonable for that reason. I'm also not sure 
> whether this, as is, is addressing a pressing use case -- for Clang 
> developers, existing non-invasive profiling tools (such as linux-perftools) 
> are likely to work a lot better for identifying where in the Clang source 
> code we're spending time. And Clang users typically don't care which function 
> we're in (unless they're filing a bug, where again a profiler is probably a 
> better tool).
> 
> However, I do think we could make `-ftime-report` vastly more useful to Clang 
> users. Specifically, I think the useful, actionable feedback we can give to 
> users would be to tell them which parts of //their source code// are taking a 
> long time to compile. Profiling information that can describe -- for instance 
> -- the time spent instantiating the N slowest templates, or handling the N 
> slowest functions, or evaluating the N slowest constant expressions, or 
> parsing the N slowest `#include`d files, seems like it would be incredibly 
> valuable. To make that work, I think we'll need to teach LLVM's timer 
> infrastructure to properly separate out "self" time from "children" time for 
> timers in the same group, and may need other infrastructure improvements.

I disagreed up until the last paragraph :)

That's exactly the crux of what most users need to know -- which parts
of my source code are causing the biggest build slow-down? The summary
information from -ftime-report can give a hint, but a detailed
breakdown would of course be great!


https://reviews.llvm.org/D43578



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

[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added subscribers: lebedev.ri, kimgr.
kimgr added a comment.

I have to say I disagree that either the nested struct/function or macros
(in any form) should count toward a function's total variable count.

Both are valid forms of abstraction, and they both remove complexity from
the containing function since they factor details *out of the function's
immediate lexical contents* (I avoid 'scope' as macros do pollute the
scope) in a way that improves readability.

There are cases where macros can make things more complex but it seems
unfair to flag variables declared by macros as making expanding functions
more complex.

In short, I think I agree with Aaron's last example classifications.

- Kim

Den tors 22 mars 2018 14:56Eugene Zelenko via Phabricator via cfe-commits <
cfe-commits@lists.llvm.org> skrev:

> Eugene.Zelenko added inline comments.
> 
> 
>  Comment at: docs/ReleaseNotes.rst:127
> 
> +- Added `VariableThreshold` option to `readability-function-size
>  +  <
>  
> http://clang.llvm.org/extra/clang-tidy/checks/readability-function-size.html>`_
> check
> 
>  
> 
> Please rebase from trunk and use //:doc:// for link.
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D44602
> 
>  ___
> 
> cfe-commits mailing list
>  cfe-commits@lists.llvm.org
>  http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44602



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

[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Peanut gallery observation: there was a discussion on the Boost list years and 
years ago where someone made the case that `if (x != nullptr) delete x;` was 
measurably faster than just calling `delete x;` I can't find it now, but I 
think it might have been in the context of their `checked_delete` library. 
Anyway, the reasoning was that with an external nullptr check, you'd pay for 
one comparison, but without it you'd always pay for a jump + a comparison. I 
suppose that only holds true for null pointers, for non-null pointers the extra 
check is just waste.

It looks to me like the compiler inserts an external null check, and you're now 
removing it in select cases, did I understand that right? I wonder if this 
could have negative effects for frequent deletion of nullptrs (e.g. a 
sometimes-allocated member of a heavily used value type).

That said, I'm not sure how valid the observation back then still is.


Repository:
  rC Clang

https://reviews.llvm.org/D43430



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

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.
Herald added a subscriber: kristof.beyls.

Ping! It would be nice to get some traction on this.


https://reviews.llvm.org/D31696



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

[PATCH] D41809: Clang counterpart change for buzzer FreeBSD support

[Kim Gräsman via Phabricator via cfe-commits]

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

Typo in the commit title: buzzer :)


Repository:
  rC Clang

https://reviews.llvm.org/D41809



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

[PATCH] D39360: [C++11] Don't put empty quotes in static_assert diagnostic.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Why? It seems easier to me to map back if the diagnostic mirrors the code 
as-written.


https://reviews.llvm.org/D39360



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

[PATCH] D36156: [rename] Introduce symbol occurrences

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I was about to suggest SymbolLocation or even SymbolLoc, but it appears to keep 
track of more than locations. "Reference" sounds a little open-ended. No more 
ideas here, I'm afraid.


Repository:
  rL LLVM

https://reviews.llvm.org/D36156



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

[PATCH] D36156: [rename] Introduce symbol occurrences

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

High-level comment from the peanut gallery:

The word "occurrences" is horrible to type and most people misspell it (you did 
great here!) Would you consider something like "SymbolMatches" or even 
"SymbolHits"?


Repository:
  rL LLVM

https://reviews.llvm.org/D36156



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

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Any chance you could try my example too? It seems like this approach might 
catch it.

But does the new approach really do anything different for your original 
example?


https://reviews.llvm.org/D35783



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

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I've seen this in the wild:

  #define LOG(m) \
  {  \
std::ostringstream os;  \
os << m << "\n";  \
LogWrite(os.str());  \
  }
  
  auto os = GetOSName();
  LOG("The OS is " << os);

It looks like your patch would miss this case. Not sure if current Clang 
catches it either, though, I don't remember the exact symptoms when we found 
this.


https://reviews.llvm.org/D35783



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

[PATCH] D31697: Check for null before using TUScope

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

> only for the function templates that use Microsoft intrinsics (e.g. 
> _BitScanForward in TrailingZerosCounter.)
>  So there's something in the parsing of builtins/intrinsics that requires 
> TUScope to be non-null.

For posterity, this was misdiagnosed on my part. It turns out the pp 
conditionals in MathExtras.h select the GCC-style intrinsics for Clang, even in 
MS compat mode (because `_MSC_VER` is consistently checked *after* 
`__has_builtin` and friends).

So the problem is really with GCC builtins inside function templates. Here's a 
minimal repro:

  template 
  int myctz(unsigned int value) {
   return __builtin_ctz(value);
  }


https://reviews.llvm.org/D31697



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

[PATCH] D31697: Check for null before using TUScope

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I did some more debugging today. This happens when we attempt to analyze 
llvm/Support/MathExtras.h, and only for the function templates that use 
Microsoft intrinsics (e.g. `_BitScanForward` in `TrailingZerosCounter`.) So 
there's something in the parsing of builtins/intrinsics that requires `TUScope` 
to be non-null.

Can we seed Sema with a valid `TUScope` before invoking `LateTemplateParser`, 
and if so, how? Or is this because we invoke the parser multiple times? I'm 
guessing Clang is already done parsing when we invoke the late template parsing.

Grateful for any ideas here.


https://reviews.llvm.org/D31697



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

[PATCH] D31697: Check for null before using TUScope

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

We'd love to see this addressed, either in our code or in Clang. But we're not 
sure what to do on our end, so... a gentle ping for help!


https://reviews.llvm.org/D31697



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

[PATCH] D32480: [clang-format] Add BinPackNamespaces option

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

We have a large closed-source codebase with this style, and would welcome 
clang-format support.


https://reviews.llvm.org/D32480



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

[PATCH] D32696: More detailed docs for UsingShadowDecl

[Kim Gräsman via Phabricator via cfe-commits]

kimgr created this revision.

I couldn't make sense of the docs before, so I sent a question to cfe-dev. 
Richard was gracious enough to fill in the blanks:
http://lists.llvm.org/pipermail/cfe-dev/2017-April/053687.html

I've tried to transfer some of his response to the Doxygen for UsingShadowDecl. 
Here's to hoping I captured everything correctly.


Repository:
  rL LLVM

https://reviews.llvm.org/D32696

Files:
  include/clang/AST/DeclCXX.h


Index: include/clang/AST/DeclCXX.h
===
--- include/clang/AST/DeclCXX.h
+++ include/clang/AST/DeclCXX.h
@@ -2856,18 +2856,34 @@
 };
 
 /// \brief Represents a shadow declaration introduced into a scope by a
-/// (resolved) using declaration.
+/// (resolved) using declaration, whose target is made visible for name lookup.
 ///
 /// For example,
 /// \code
 /// namespace A {
-///   void foo();
+///   void foo(int);
+///   void foo(double);
 /// }
 /// namespace B {
 ///   using A::foo; // <- a UsingDecl
-/// // Also creates a UsingShadowDecl for A::foo() in B
+/// // Also creates UsingShadowDecls for both A::foo(int) and
+/// // A::foo(double) in B
 /// }
 /// \endcode
+///
+/// Derived class member declarations can hide any shadow declarations brought
+/// in by a class-scope UsingDecl;
+/// \code
+/// class Base {
+/// public:
+///   void foo();
+/// };
+/// class Derived : public Base {
+/// public:
+///   using Base::foo; // <- a UsingDecl
+///   void foo();  // Hides Base::foo and suppresses its UsingShadowDecl.
+/// };
+/// \endcode
 class UsingShadowDecl : public NamedDecl, public Redeclarable 
{
   void anchor() override;
 


Index: include/clang/AST/DeclCXX.h
===
--- include/clang/AST/DeclCXX.h
+++ include/clang/AST/DeclCXX.h
@@ -2856,18 +2856,34 @@
 };
 
 /// \brief Represents a shadow declaration introduced into a scope by a
-/// (resolved) using declaration.
+/// (resolved) using declaration, whose target is made visible for name lookup.
 ///
 /// For example,
 /// \code
 /// namespace A {
-///   void foo();
+///   void foo(int);
+///   void foo(double);
 /// }
 /// namespace B {
 ///   using A::foo; // <- a UsingDecl
-/// // Also creates a UsingShadowDecl for A::foo() in B
+/// // Also creates UsingShadowDecls for both A::foo(int) and
+/// // A::foo(double) in B
 /// }
 /// \endcode
+///
+/// Derived class member declarations can hide any shadow declarations brought
+/// in by a class-scope UsingDecl;
+/// \code
+/// class Base {
+/// public:
+///   void foo();
+/// };
+/// class Derived : public Base {
+/// public:
+///   using Base::foo; // <- a UsingDecl
+///   void foo();  // Hides Base::foo and suppresses its UsingShadowDecl.
+/// };
+/// \endcode
 class UsingShadowDecl : public NamedDecl, public Redeclarable {
   void anchor() override;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32294: [clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Python 3 concern inline




Comment at: clang-tidy/tool/run-clang-tidy.py:100
+  except:
+print >>sys.stderr, "Unable to run clang-apply-replacements."
+sys.exit(1)

alexfh wrote:
> "Unable to run clang-apply-replacements" without any details seems to be far 
> worse than just a default stack trace from an unhandled exception. At the 
> very least add the message from the exception.
I don't know what the general Python3 ambitions of this script is, but it would 
be nice if the new code didn't use the Python2-only print style.

You can:

from __future__ import print_function

at the top of the file, and then use Python3-style print:

print("Unable to run clang-apply-replacements", file=sys.stderr)


https://reviews.llvm.org/D32294



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

[PATCH] D31697: Check for null before using TUScope

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

For some context, the backtrace when this happens is:

 include-what-you-use.exe!clang::Scope::getEntity() Line 313C++

include-what-you-use.exe!clang::Sema::PushOnScopeChains(clang::NamedDecl * 
D=0x07ed05b0, clang::Scope * S=0x, bool AddToContext=true) Line 1302 C++

include-what-you-use.exe!clang::Sema::LazilyCreateBuiltin(clang::IdentifierInfo 
* II=0x008087f0, unsigned int ID=0x0114, clang::Scope * S=0x, bool 
ForRedeclaration=false, clang::SourceLocation Loc={...}) Line 1910   C++
include-what-you-use.exe!LookupBuiltin(clang::Sema & S={...}, 
clang::LookupResult & R={...}) Line 699   C++
include-what-you-use.exe!LookupDirect(clang::Sema & S={...}, 
clang::LookupResult & R={...}, const clang::DeclContext * DC=0x007c1a14) Line 
850  C++
include-what-you-use.exe!CppNamespaceLookup(clang::Sema & S={...}, 
clang::LookupResult & R={...}, clang::ASTContext & Context={...}, 
clang::DeclContext * NS=0x007c1a14, 
`anonymous-namespace'::UnqualUsingDirectiveSet & UDirs={...}) Line 931 C++
include-what-you-use.exe!clang::Sema::CppLookupName(clang::LookupResult 
& R={...}, clang::Scope * S=0x0078c870) Line 1310   C++
include-what-you-use.exe!clang::Sema::LookupName(clang::LookupResult & 
R={...}, clang::Scope * S=0x079fcd10, bool AllowBuiltinCreation=false) Line 
1798 C++
include-what-you-use.exe!clang::Sema::getTypeName(const 
clang::IdentifierInfo & II={...}, clang::SourceLocation NameLoc={...}, 
clang::Scope * S=0x079fcd10, clang::CXXScopeSpec * SS=0x0449db94, bool 
isClassName=false, bool HasTrailingDot=false, clang::OpaquePtr 
ObjectTypePtr={...}, bool IsCtorOrDtorName=false, bool 
WantNontrivialTypeSourceInfo=true, bool IsClassTemplateDeductionContext=true, 
clang::IdentifierInfo * * CorrectedII=0x) Line 331   C++

include-what-you-use.exe!clang::Parser::TryAnnotateTypeOrScopeTokenAfterScopeSpec(clang::CXXScopeSpec
 & SS={...}, bool IsNewScope=true) Line 1727   C++
include-what-you-use.exe!clang::Parser::TryAnnotateTypeOrScopeToken() 
Line 1717 C++
include-what-you-use.exe!clang::Parser::ParseCastExpression(bool 
isUnaryExpression=false, bool isAddressOfOperand=false, bool & 
NotCastExpr=false, clang::Parser::TypeCastState isTypeCast=NotTypeCast, bool 
isVectorLiteral=false) Line 876C++
include-what-you-use.exe!clang::Parser::ParseCastExpression(bool 
isUnaryExpression=false, bool isAddressOfOperand=false, 
clang::Parser::TypeCastState isTypeCast=NotTypeCast, bool 
isVectorLiteral=false) Line 484  C++

include-what-you-use.exe!clang::Parser::ParseAssignmentExpression(clang::Parser::TypeCastState
 isTypeCast=NotTypeCast) Line 171 C++

include-what-you-use.exe!clang::Parser::ParseExpression(clang::Parser::TypeCastState
 isTypeCast=NotTypeCast) Line 121   C++
include-what-you-use.exe!clang::Parser::ParseReturnStatement() Line 
1905C++

include-what-you-use.exe!clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector & Stmts={...}, clang::Parser::AllowedConstructsKind Allowed=ACK_Any, 
clang::SourceLocation * TrailingElseLoc=0x, 
clang::Parser::ParsedAttributesWithRange & Attrs={...}) Line 265C++

include-what-you-use.exe!clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector & Stmts={...}, clang::Parser::AllowedConstructsKind Allowed=ACK_Any, 
clang::SourceLocation * TrailingElseLoc=0x) Line 113   C++
include-what-you-use.exe!clang::Parser::ParseCompoundStatementBody(bool 
isStmtExpr=false) Line 996  C++

include-what-you-use.exe!clang::Parser::ParseFunctionStatementBody(clang::Decl 
* Decl=0x07e84ce0, clang::Parser::ParseScope & BodyScope={...}) Line 1965   
 C++

include-what-you-use.exe!clang::Parser::ParseLateTemplatedFuncDef(clang::LateParsedTemplate
 & LPT={...}) Line 1418  C++
include-what-you-use.exe!clang::Parser::LateTemplateParserCallback(void 
* P=0x0077d438, clang::LateParsedTemplate & LPT={...}) Line 1339C++

include-what-you-use.exe!include_what_you_use::IwyuAstConsumer::ParseFunctionTemplates(clang::TranslationUnitDecl
 * decl=0x007c1a00) Line 3558  C++

include-what-you-use.exe!include_what_you_use::IwyuAstConsumer::HandleTranslationUnit(clang::ASTContext
 & context={...}) Line 3503  C++
include-what-you-use.exe!clang::ParseAST(clang::Sema & S={...}, bool 
PrintStats=false, bool SkipFunctionBodies=false) Line 159  C++
include-what-you-use.exe!clang::ASTFrontendAction::ExecuteAction() Line 
611 C++
include-what-you-use.exe!clang::FrontendAction::Execute() Line 512  
C++

include-what-you-use.exe!clang::CompilerInstance::ExecuteAction(clang::FrontendAction
 & Act={...}) Line 970 C++

This is triggered when we invoke a hacky hook in Sema to force instantiation of 
late-parsed function templates:

  void 

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: clang/tools/CMakeLists.txt:35
+# if include-what-you-use is cloned for building in-tree, add it here.
+if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/include-what-you-use")
+  add_clang_subdirectory(include-what-you-use)

beanz wrote:
> rnk wrote:
> > Maybe we should do `llvm_add_implicit_projects(CLANG)` here instead?
> > 
> > Or do we not want clang/tools to be a project extension point? Would IWYU 
> > build fine if we added it to llvm/projects or llvm/tools? Maybe we should 
> > just recommend that.
> Either `llvm_add_implicit_projects(CLANG)` or 
> `add_llvm_external_project(include-what-you-use include-what-you-use)`.
> 
> The former case would make this a generic extension point, which I think is 
> good, the later would add just this project without needing to wrap it in an 
> `if`.
> 
> I would prefer going the implicit route because I actually have a distaste 
> for having code living in-tree that is specifically for supporting 
> out-of-tree code.
> Would IWYU build fine if we added it to llvm/projects or llvm/tools? Maybe we 
> should just recommend that.

IWYU has all dependencies explicitly listed to support out-of-tree builds too, 
so I think it could be made to work with some include/lib path setup. But it 
would feel weird layering-wise, since it depends so heavily on clang.

As for `llvm_add_implicit_projects` vs `add_llvm_external_project`, I'm 
guessing it doesn't make a difference for IWYU's CMake system? Mild preference 
for `llvm_add_implicit_projects` to keep mention of `include-what-you-use` out 
of Clang until we can form a coherent story about upstreaming.


https://reviews.llvm.org/D31696



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

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

> BTW, kimgr@, is there any particular reason you haven't tried to upstream the 
> tool?
>  Maybe even as not a standalone tool but as a set of checks in clang-tidy.

Lack of time, mostly. IWYU was written before there was libtooling, and follows 
Google style, so we've felt it would be an odd bird in clang-tools-extra, for 
example.


https://reviews.llvm.org/D31696



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

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

As an include-what-you-use maintainer, I would very much welcome this. I've 
been too shy to suggest it myself ;-).


https://reviews.llvm.org/D31696



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

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

A few more 'inhertiance' left, otherwise spelling looks good to me :-)




Comment at: lib/Format/TokenAnnotator.cpp:679
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInhertianceList)
+Tok->Type = TT_InheritanceComma;

This still says 'Inhertiance'



Comment at: lib/Format/TokenAnnotator.cpp:950
 bool InCtorInitializer = false;
+bool InInhertianceList = false;
 bool CaretFound = false;

'Inhertiance'


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



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

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Oops, the flag is called `BreakBeforeInhertianceComma` with a misspelling 
throughout. May want to search for `inhertiance` throughout and see if there 
are more cases.




Comment at: docs/ClangFormatStyleOptions.rst:531
 
+**BreakBeforeInhertianceComma** (``bool``)
+  If ``true``, in the class inheritance expression clang-format will

Tyop: Inhertiance


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



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

[PATCH] D30773: Make git-clang-format python 3 compatible

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: tools/clang-format/git-clang-format:323
   allowed_extensions = frozenset(allowed_extensions)
-  for filename in dictionary.keys():
+  for filename in list(dictionary.keys()):
 base_ext = filename.rsplit('.', 1)

EricWF wrote:
> EricWF wrote:
> > kimgr wrote:
> > > This should not be necessary for iteration -- in Python3 it returns a 
> > > generator instead of a list, but generators can be iterated.
> > This was done by the 2to3 tool. I'll have to see what it thinks it was up 
> > to. But I agree this seems wrong.
> Ah, so this is needed because in python 3 it's a runtime error to remove 
> items from a dictionary while iterating over the keys. So we have to make a 
> copy first.
Oh, I see, I didn't catch that. Carry on.


https://reviews.llvm.org/D30773



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

[PATCH] D30773: Make git-clang-format python 3 compatible

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Some nits from a Python3 hacker.




Comment at: tools/clang-format/git-clang-format:143
   for filename in ignored_files:
-print '   ', filename
+print('   ', filename)
 if changed_lines:

I find `print('template', tail)` surprising in Python3, but it could be because 
we don't use it in our local standards. I'd jump immediately to formatting to 
make this 2/3-compatible:

print('%s' % filename)

or in this case maybe just join the strings:

print('' + filename)




Comment at: tools/clang-format/git-clang-format:323
   allowed_extensions = frozenset(allowed_extensions)
-  for filename in dictionary.keys():
+  for filename in list(dictionary.keys()):
 base_ext = filename.rsplit('.', 1)

This should not be necessary for iteration -- in Python3 it returns a generator 
instead of a list, but generators can be iterated.



Comment at: tools/clang-format/git-clang-format:491
 if unstaged_files:
-  print >>sys.stderr, ('The following files would be modified but '
-   'have unstaged changes:')
-  print >>sys.stderr, unstaged_files
-  print >>sys.stderr, 'Please commit, stage, or stash them first.'
+  print(('The following files would be modified but '
+   'have unstaged changes:'), file=sys.stderr)

No need for parentheses around the string



Comment at: tools/clang-format/git-clang-format:492
+  print(('The following files would be modified but '
+   'have unstaged changes:'), file=sys.stderr)
+  print(unstaged_files, file=sys.stderr)

I wonder if `file=sys.stderr` works with Python 2.7's print statement. Have you 
tested this with 2.7?

You can use `__future__` to get the print function behavior in 2.7 as well:
http://stackoverflow.com/questions/32032697/how-to-use-from-future-import-print-function



Comment at: tools/clang-format/git-clang-format:512-515
+def to_string(bytes):
+if isinstance(bytes, str):
+return bytes
+return to_bytes(bytes)

This looks wrong -- where does `to_bytes` come from?

I can never get my head around Python2's string/bytes philosophy. In Python3 
you would do:

# stdout and stderr are always `bytes`
stdout = stdout.decode('utf-8')
stderr = stderr.decode('utf-8')

(assuming the input *is* utf-8)

Not sure how that plays with Python2, but the current code doesn't look right.



https://reviews.llvm.org/D30773



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

[PATCH] D30532: Add examples to clang-format configuration

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

For what it's worth, I also find this useful to be able to see at a glance what 
the setting does. I know I've tried several different settings in the past 
before finding the one I was looking for.


https://reviews.llvm.org/D30532



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

[PATCH] D30564: [clang-tidy] Format code around applied fixes

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.



Comment at: docs/clang-tidy/index.rst:184
 -p=  - Build path
+-quiet   -
+   Run clang-tidy in quiet mode. This 
suppresses

This looks like a separate patch?


https://reviews.llvm.org/D30564



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