[PATCH] D158964: [clangd] support folding multi-line `#include`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158964

Files:
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp


Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -453,6 +453,31 @@
 << Test;
   }
 }
+
+TEST(FoldingRanges, Includes) {
+  const char *Test = R"cpp(
+[[#include 
+#include 
+#include ]]
+
+#include 
+
+[[#include "external/Logger.h"
+#include "external/Vector.h"
+#include "project/DataStructures/Trie.h"]]
+
+[[#include "project/File.h"
+#include 
+#include ]]
+
+#include "math.h"
+  )cpp";
+  auto T = Annotations{Test};
+  EXPECT_THAT(gatherFoldingRanges(
+  llvm::cantFail(getFoldingRanges(T.code().str(), false))),
+  UnorderedElementsAreArray(T.ranges()))
+  << Test;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/SemanticSelection.cpp
===
--- clang-tools-extra/clangd/SemanticSelection.cpp
+++ clang-tools-extra/clangd/SemanticSelection.cpp
@@ -175,7 +175,32 @@
   return collectFoldingRanges(SyntaxTree, TM);
 }
 
-// FIXME( usaxena95): Collect PP conditional regions, includes and other code
+namespace {
+struct IncludeCollector {
+  explicit IncludeCollector(const pseudo::DirectiveTree ) { walk(T); }
+
+  void walk(const pseudo::DirectiveTree ) {
+for (const auto  : T.Chunks)
+  std::visit(*this, C);
+  }
+
+  void operator()(const pseudo::DirectiveTree::Code &) {}
+
+  void operator()(const pseudo::DirectiveTree::Directive ) {
+if (D.Kind == tok::pp_include)
+  Results.push_back(D);
+  }
+
+  void operator()(const pseudo::DirectiveTree::Conditional ) {
+if (C.Taken)
+  walk(C.Branches[*C.Taken].second);
+  }
+
+  std::vector Results;
+};
+} // namespace
+
+// FIXME( usaxena95): Collect PP conditional regions and other code
 // regions (e.g. public/private/protected sections of classes, control flow
 // statement bodies).
 // Related issue: https://github.com/clangd/clangd/issues/310
@@ -267,6 +292,20 @@
 }
 AddFoldingRange(Start, End, FoldingRange::COMMENT_KIND);
   }
+  // Multi-line `#include`
+  auto Includes = IncludeCollector{DirectiveStructure}.Results;
+  for (auto It = Includes.begin(); It != Includes.end();) {
+Position Start = StartPosition(OrigStream.tokens(It->Tokens).front());
+Position End = EndPosition(OrigStream.tokens(It->Tokens).back());
+It++;
+while (It != Includes.end() &&
+   StartPosition(OrigStream.tokens(It->Tokens).front()).line ==
+   End.line + 1) {
+  End = EndPosition(OrigStream.tokens(It->Tokens).back());
+  It++;
+}
+AddFoldingRange(Start, End, FoldingRange::IMPORT_KIND);
+  }
   return Result;
 }
 


Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -453,6 +453,31 @@
 << Test;
   }
 }
+
+TEST(FoldingRanges, Includes) {
+  const char *Test = R"cpp(
+[[#include 
+#include 
+#include ]]
+
+#include 
+
+[[#include "external/Logger.h"
+#include "external/Vector.h"
+#include "project/DataStructures/Trie.h"]]
+
+[[#include "project/File.h"
+#include 
+#include ]]
+
+#include "math.h"
+  )cpp";
+  auto T = Annotations{Test};
+  EXPECT_THAT(gatherFoldingRanges(
+  llvm::cantFail(getFoldingRanges(T.code().str(), false))),
+  UnorderedElementsAreArray(T.ranges()))
+  << Test;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/SemanticSelection.cpp
===
--- clang-tools-extra/clangd/SemanticSelection.cpp
+++ clang-tools-extra/clangd/SemanticSelection.cpp
@@ -175,7 +175,32 @@
   return collectFoldingRanges(SyntaxTree, TM);
 }
 
-// FIXME( usaxena95): Collect PP conditional regions, includes and other code
+namespace {
+struct IncludeCollector {
+  explicit IncludeCollector(const pseudo::DirectiveTree ) { walk(T); }
+
+  void walk(const pseudo::DirectiveTree ) {
+for (const auto  : T.Chunks)
+  std::visit(*this, C);
+  }
+
+  void operator()(const pseudo::DirectiveTree::Code &) {}
+
+  

[PATCH] D157956: [clangd] don't add inlay hint for dependent type in structured binding

[Vincent Hong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdc10bd43a103: [clangd] dont add inlay hint for 
dependent type in structured binding (authored by v1nh1shungry).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157956

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1353,6 +1353,11 @@
   // FIXME: It would be nice to show "T" as the hint.
   auto $var2[[var2]] = arg;
 }
+
+template 
+void bar(T arg) {
+  auto [a, b] = arg;
+}
   )cpp");
 }
 
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -733,7 +733,8 @@
 // For structured bindings, print canonical types. This is important
 // because for bindings that use the tuple_element protocol, the
 // non-canonical types would be "tuple_element::type".
-if (auto Type = Binding->getType(); !Type.isNull())
+if (auto Type = Binding->getType();
+!Type.isNull() && !Type->isDependentType())
   addTypeHint(Binding->getLocation(), Type.getCanonicalType(),
   /*Prefix=*/": ");
   }


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1353,6 +1353,11 @@
   // FIXME: It would be nice to show "T" as the hint.
   auto $var2[[var2]] = arg;
 }
+
+template 
+void bar(T arg) {
+  auto [a, b] = arg;
+}
   )cpp");
 }
 
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -733,7 +733,8 @@
 // For structured bindings, print canonical types. This is important
 // because for bindings that use the tuple_element protocol, the
 // non-canonical types would be "tuple_element::type".
-if (auto Type = Binding->getType(); !Type.isNull())
+if (auto Type = Binding->getType();
+!Type.isNull() && !Type->isDependentType())
   addTypeHint(Binding->getLocation(), Type.getCanonicalType(),
   /*Prefix=*/": ");
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157956: [clangd] don't add inlay hint for dependent type in structured binding

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thank you for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157956

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

[PATCH] D157956: [clangd] don't add inlay hint for dependent type in structured binding

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added a reviewer: nridge.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Currently clangd will display useless inlay hint for dependent type in
structured binding, e.g.

  template 
  void foobar(T arg) {
auto [a/*: */, b/*: */] = arg;
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157956

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1333,6 +1333,11 @@
   // FIXME: It would be nice to show "T" as the hint.
   auto $var2[[var2]] = arg;
 }
+
+template 
+void bar(T arg) {
+  auto [a, b] = arg;
+}
   )cpp");
 }
 
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -676,7 +676,8 @@
 // For structured bindings, print canonical types. This is important
 // because for bindings that use the tuple_element protocol, the
 // non-canonical types would be "tuple_element::type".
-if (auto Type = Binding->getType(); !Type.isNull())
+if (auto Type = Binding->getType();
+!Type.isNull() && !Type->isDependentType())
   addTypeHint(Binding->getLocation(), Type.getCanonicalType(),
   /*Prefix=*/": ");
   }


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1333,6 +1333,11 @@
   // FIXME: It would be nice to show "T" as the hint.
   auto $var2[[var2]] = arg;
 }
+
+template 
+void bar(T arg) {
+  auto [a, b] = arg;
+}
   )cpp");
 }
 
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -676,7 +676,8 @@
 // For structured bindings, print canonical types. This is important
 // because for bindings that use the tuple_element protocol, the
 // non-canonical types would be "tuple_element::type".
-if (auto Type = Binding->getType(); !Type.isNull())
+if (auto Type = Binding->getType();
+!Type.isNull() && !Type->isDependentType())
   addTypeHint(Binding->getLocation(), Type.getCanonicalType(),
   /*Prefix=*/": ");
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

In D139705#4216539 , @erichkeane 
wrote:

> In D139705#4216530 , 
> @tomasz-kaminski-sonarsource wrote:
>
>>> Since the motivation for this patch here was "make sure we're pointing to 
>>> the 'end' so we can suggest an init fixit", perhaps this SHOULD be changed 
>>> to the 'end' still, but just fix the case where the initializer was 
>>> omitted.  So
>>>
>>>   /// What USED to happen:
>>>   template<> double temp = 1;
>>>   //End is here:   ^
>>>   template<> double temp;
>>>   //End is here:   ^
>>>   
>>>   //What is happening now:
>>>   template<> double temp = 1;
>>>   //End is here:   ^
>>>   template<> double temp;
>>>   //End is here:   ^
>>>   
>>>   // What I think we want:
>>>   template<> double temp = 1;
>>>   //End is here:   ^
>>>   template<> double temp;
>>>   //End is here:   ^
>>>
>>> Right?  So this isn't so much as a separate function, its just to make sure 
>>> we get the 'source range' to include 'everything', including the 
>>> initializer, if present.
>>
>> Indeed, this would address our concern, and allow properly inserting 
>> initializer. This would build down to repeating the condition from here: 
>> https://github.com/llvm/llvm-project/blob/bbe1394c9602ab9a939d9b17199d5f538cac9d0c/clang/lib/AST/Decl.cpp#L1207.
>>
>> I was suggesting adding an additional function, as it would cover additional 
>> use cases like replacing or removing the initializer, and then 
>> `VarDecl::getSourceRange` could be defined as:
>>
>>   SourceRange VarDecl::getSourceRange() const {
>> if (const Expr *Init = getInit()) {
>>   SourceLocation InitEnd = Init->getEndLoc();
>>   // If Init is implicit, ignore its source range and fallback on
>>   // DeclaratorDecl::getSourceRange() to handle postfix elements.
>>   if (InitEnd.isValid() && InitEnd != getLocation())
>> return SourceRange(getOuterLocStart(), InitEnd);
>> }
>> return getDeclatorRange();
>>   }
>
> That would make sense to me.  Feel free to submit a patch (assuming 
> @v1nh1shungry doesn't respond/get to it first).

Thank you for the suggestion! This makes sense to me too. But I'm sorry for 
being busy recently. Please feel free to submit a patch. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb252824e6e6e: [clangd] fix wrong CalleeArgInfo in the hover 
(authored by v1nh1shungry).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -900,6 +900,27 @@
  HI.CalleeArgInfo->Type = "int &";
  HI.CallPassType = HoverInfo::PassType{PassMode::Ref, false};
}},
+  {
+  R"cpp(
+  void foobar(const float );
+  int main() {
+int a = 0;
+foobar([[^a]]);
+  }
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Definition = "int a = 0";
+HI.LocalScope = "main::";
+HI.Value = "0";
+HI.Type = "int";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "arg";
+HI.CalleeArgInfo->Type = "const float &";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, true};
+  }},
   {// Literal passed to function call
R"cpp(
   void fun(int arg_a, const int _b) {};
@@ -934,6 +955,38 @@
  HI.CalleeArgInfo->Type = "const int &";
  HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, false};
}},
+  {
+  R"cpp(
+int add(int lhs, int rhs);
+int main() {
+  add(1 [[^+]] 2, 3);
+}
+)cpp",
+  [](HoverInfo ) {
+HI.Name = "expression";
+HI.Kind = index::SymbolKind::Unknown;
+HI.Type = "int";
+HI.Value = "3";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "lhs";
+HI.CalleeArgInfo->Type = "int";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, false};
+  }},
+  {
+  R"cpp(
+void foobar(const float );
+int main() {
+  foobar([[^0]]);
+}
+)cpp",
+  [](HoverInfo ) {
+HI.Name = "literal";
+HI.Kind = index::SymbolKind::Unknown;
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "arg";
+HI.CalleeArgInfo->Type = "const float &";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, true};
+  }},
   {// Extra info for method call.
R"cpp(
   class C {
@@ -960,6 +1013,29 @@
  HI.CalleeArgInfo->Default = "3";
  HI.CallPassType = HoverInfo::PassType{PassMode::Value, false};
}},
+  {
+  R"cpp(
+  struct Foo {
+Foo(const int &);
+  };
+  void foo(Foo);
+  void bar() {
+const int x = 0;
+foo([[^x]]);
+  }
+   )cpp",
+  [](HoverInfo ) {
+HI.Name = "x";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Definition = "const int x = 0";
+HI.LocalScope = "bar::";
+HI.Value = "0";
+HI.Type = "const int";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Type = "Foo";
+HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, true};
+  }},
   {// Dont crash on invalid decl
R"cpp(
 // error-ok
@@ -1673,8 +1749,8 @@
   }},
   {
   R"cpp(// Function definition via using declaration
-namespace ns { 
-  void foo(); 
+namespace ns {
+  void foo();
 }
 int main() {
   using ns::foo;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -952,6 +952,15 @@
   }
 }
 
+HoverInfo::PassType::PassMode getPassMode(QualType ParmType) {
+  if (ParmType->isReferenceType()) {
+if (ParmType->getPointeeType().isConstQualified())
+  return HoverInfo::PassType::ConstRef;
+return HoverInfo::PassType::Ref;
+  }
+  return HoverInfo::PassType::Value;
+}
+
 // If N is passed as argument to a function, fill HI.CalleeArgInfo with
 // information about that argument.
 void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo ,
@@ -972,14 +981,19 @@
   if (!FD || FD->isOverloadedOperator() || FD->isVariadic())
 return;
 
+  HoverInfo::PassType PassType;
+
   // Find argument index for N.
   for (unsigned I = 0; I < CE->getNumArgs() && I < 

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thanks for the review! @kadircet

Would you mind having a look at if there're any concerns about the current code 
change, @nridge, @tom-anders, and @adamcz? Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 493626.
v1nh1shungry added a comment.

- restore code changes
- add test cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -900,6 +900,27 @@
  HI.CalleeArgInfo->Type = "int &";
  HI.CallPassType = HoverInfo::PassType{PassMode::Ref, false};
}},
+  {
+  R"cpp(
+  void foobar(const float );
+  int main() {
+int a = 0;
+foobar([[^a]]);
+  }
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Definition = "int a = 0";
+HI.LocalScope = "main::";
+HI.Value = "0";
+HI.Type = "int";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "arg";
+HI.CalleeArgInfo->Type = "const float &";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, true};
+  }},
   {// Literal passed to function call
R"cpp(
   void fun(int arg_a, const int _b) {};
@@ -934,6 +955,38 @@
  HI.CalleeArgInfo->Type = "const int &";
  HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, false};
}},
+  {
+  R"cpp(
+int add(int lhs, int rhs);
+int main() {
+  add(1 [[^+]] 2, 3);
+}
+)cpp",
+  [](HoverInfo ) {
+HI.Name = "expression";
+HI.Kind = index::SymbolKind::Unknown;
+HI.Type = "int";
+HI.Value = "3";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "lhs";
+HI.CalleeArgInfo->Type = "int";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, false};
+  }},
+  {
+  R"cpp(
+void foobar(const float );
+int main() {
+  foobar([[^0]]);
+}
+)cpp",
+  [](HoverInfo ) {
+HI.Name = "literal";
+HI.Kind = index::SymbolKind::Unknown;
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "arg";
+HI.CalleeArgInfo->Type = "const float &";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, true};
+  }},
   {// Extra info for method call.
R"cpp(
   class C {
@@ -960,6 +1013,29 @@
  HI.CalleeArgInfo->Default = "3";
  HI.CallPassType = HoverInfo::PassType{PassMode::Value, false};
}},
+  {
+  R"cpp(
+  struct Foo {
+Foo(const int &);
+  };
+  void foo(Foo);
+  void bar() {
+const int x = 0;
+foo([[^x]]);
+  }
+   )cpp",
+  [](HoverInfo ) {
+HI.Name = "x";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Definition = "const int x = 0";
+HI.LocalScope = "bar::";
+HI.Value = "0";
+HI.Type = "const int";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Type = "Foo";
+HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, true};
+  }},
   {// Dont crash on invalid decl
R"cpp(
 // error-ok
@@ -1673,8 +1749,8 @@
   }},
   {
   R"cpp(// Function definition via using declaration
-namespace ns { 
-  void foo(); 
+namespace ns {
+  void foo();
 }
 int main() {
   using ns::foo;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -952,6 +952,15 @@
   }
 }
 
+HoverInfo::PassType::PassMode getPassMode(QualType ParmType) {
+  if (ParmType->isReferenceType()) {
+if (ParmType->getPointeeType().isConstQualified())
+  return HoverInfo::PassType::ConstRef;
+return HoverInfo::PassType::Ref;
+  }
+  return HoverInfo::PassType::Value;
+}
+
 // If N is passed as argument to a function, fill HI.CalleeArgInfo with
 // information about that argument.
 void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo ,
@@ -972,14 +981,19 @@
   if (!FD || FD->isOverloadedOperator() || FD->isVariadic())
 return;
 
+  HoverInfo::PassType PassType;
+
   // Find argument index for N.
   for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
 if (CE->getArg(I) != 

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:994
   HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+  PassType.PassBy = getPassMode(PVD->getType());
+}

kadircet wrote:
> v1nh1shungry wrote:
> > kadircet wrote:
> > > v1nh1shungry wrote:
> > > > kadircet wrote:
> > > > > v1nh1shungry wrote:
> > > > > > kadircet wrote:
> > > > > > > sorry for showing up late to the party but instead of changing 
> > > > > > > rest of the code, can we apply this logic only when there are no 
> > > > > > > implicit casts/conversions between the arg and callexpr (i.e `N 
> > > > > > > == `)?
> > > > > > To make sure I understand it correctly, do you mean I should give 
> > > > > > up any other code changes I made but keep this logic, and put this 
> > > > > > logic into the `N == ` branch?
> > > > > > 
> > > > > > Sorry if I misunderstood anything! Shame for not being a good 
> > > > > > reader :(
> > > > > > To make sure I understand it correctly, do you mean I should give 
> > > > > > up any other code changes I made but keep this logic, and put this 
> > > > > > logic into the N ==  branch?
> > > > > 
> > > > > Somewhat.
> > > > > 
> > > > > Basically this code had the assumption that if we don't see any 
> > > > > casts/conversions between the expression creating the argument and 
> > > > > the expression getting passed to the callexpr, it must be passed by 
> > > > > reference, and this was wrong. Even before the patch that added 
> > > > > handling for literals.
> > > > > 
> > > > > The rest of the handling for casts/conversions/constructors in 
> > > > > between have been working so far and was constructed based on what 
> > > > > each particular cast does, not for specific cases hence they're 
> > > > > easier (for the lack of a better word) to reason about. Hence I'd 
> > > > > rather keep them as is, especially the changes in handling 
> > > > > `MaterializeTemporaryExpr` don't sound right. I can see the example 
> > > > > you've at hand, but we shouldn't be producing "converted" results for 
> > > > > it anyway (the AST should have a NoOp implicit cast to `const int` 
> > > > > and then a `MaterializeTemporaryExpr`, which shouldn't generated any 
> > > > > converted signals with the existing code already).
> > > > > 
> > > > > Hence the my proposal is getting rid of the assumption around "if we 
> > > > > don't see any casts/conversions between the expression creating the 
> > > > > argument and the expression getting passed to the callexpr, it must 
> > > > > be passed by reference", and use the type information in 
> > > > > `ParmVarDecl` directly when we don't have any implicit nodes in 
> > > > > between to infer `PassBy`.
> > > > > This should imply also getting rid of the special case for literals 
> > > > > (`if (isLiteral(E) && N->Parent == OuterNode.Parent)`).
> > > > > 
> > > > > Does that make sense?
> > > > Thanks for the detailed explanation! But before we go ahead here, what 
> > > > do you think about the new test case I'm talking about above? Do you 
> > > > agree with my conclusion?
> > > i suppose you mean:
> > > 
> > > ```
> > > void foobar(const float &);
> > > int main() {
> > >   foobar(0);
> > >   ^
> > > }
> > > ```
> > > 
> > > first of all the version of the patch that i propose doesn't involve any 
> > > changes in behaviour here (as we actually have an implicit cast in 
> > > between, and i am suggesting finding out passby based on type of the 
> > > parmvardecl only when there are no casts in between).
> > > 
> > > i can see @nridge 's reasoning about indicating copies by saying pass by 
> > > value vs ref, which unfortunately doesn't align with C++ semantics 
> > > directly (as what we have here is a prvalue, and it is indeed passed by 
> > > value, without any copies to the callee).
> > > 
> > > it isn't very obvious anywhere but the main functionality we wanted to 
> > > provide to the developer was help them figure out if a function call can 
> > > mutate a parameter they were passing in, therefore it didn't prioritise 
> > > literals at all. we probably should've made better wording choices in the 
> > > UI and talked about "immutability". hence from functionality point of 
> > > view calling this pass by `value` vs `const ref` doesn't make a huge 
> > > difference (but that's probably only in my mind and people are already 
> > > using it to infer other things like whether we're going to trigger 
> > > copies).
> > > 
> > > so i'd actually leave this case as-is, and think about what we're 
> > > actually trying to provide by showing arg info on literals. as it's 
> > > currently trying to overload the meaning of `passby` and causing 
> > > confusions. since the initial intent was to just convey "immutability" 
> > > one suggestion would be to just hide the `passby` information for 
> > > literals.
> > > otherwise from value categories point of view, these are always passed by 
> > 

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:994
   HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+  PassType.PassBy = getPassMode(PVD->getType());
+}

kadircet wrote:
> v1nh1shungry wrote:
> > kadircet wrote:
> > > v1nh1shungry wrote:
> > > > kadircet wrote:
> > > > > sorry for showing up late to the party but instead of changing rest 
> > > > > of the code, can we apply this logic only when there are no implicit 
> > > > > casts/conversions between the arg and callexpr (i.e `N == 
> > > > > `)?
> > > > To make sure I understand it correctly, do you mean I should give up 
> > > > any other code changes I made but keep this logic, and put this logic 
> > > > into the `N == ` branch?
> > > > 
> > > > Sorry if I misunderstood anything! Shame for not being a good reader :(
> > > > To make sure I understand it correctly, do you mean I should give up 
> > > > any other code changes I made but keep this logic, and put this logic 
> > > > into the N ==  branch?
> > > 
> > > Somewhat.
> > > 
> > > Basically this code had the assumption that if we don't see any 
> > > casts/conversions between the expression creating the argument and the 
> > > expression getting passed to the callexpr, it must be passed by 
> > > reference, and this was wrong. Even before the patch that added handling 
> > > for literals.
> > > 
> > > The rest of the handling for casts/conversions/constructors in between 
> > > have been working so far and was constructed based on what each 
> > > particular cast does, not for specific cases hence they're easier (for 
> > > the lack of a better word) to reason about. Hence I'd rather keep them as 
> > > is, especially the changes in handling `MaterializeTemporaryExpr` don't 
> > > sound right. I can see the example you've at hand, but we shouldn't be 
> > > producing "converted" results for it anyway (the AST should have a NoOp 
> > > implicit cast to `const int` and then a `MaterializeTemporaryExpr`, which 
> > > shouldn't generated any converted signals with the existing code already).
> > > 
> > > Hence the my proposal is getting rid of the assumption around "if we 
> > > don't see any casts/conversions between the expression creating the 
> > > argument and the expression getting passed to the callexpr, it must be 
> > > passed by reference", and use the type information in `ParmVarDecl` 
> > > directly when we don't have any implicit nodes in between to infer 
> > > `PassBy`.
> > > This should imply also getting rid of the special case for literals (`if 
> > > (isLiteral(E) && N->Parent == OuterNode.Parent)`).
> > > 
> > > Does that make sense?
> > Thanks for the detailed explanation! But before we go ahead here, what do 
> > you think about the new test case I'm talking about above? Do you agree 
> > with my conclusion?
> i suppose you mean:
> 
> ```
> void foobar(const float &);
> int main() {
>   foobar(0);
>   ^
> }
> ```
> 
> first of all the version of the patch that i propose doesn't involve any 
> changes in behaviour here (as we actually have an implicit cast in between, 
> and i am suggesting finding out passby based on type of the parmvardecl only 
> when there are no casts in between).
> 
> i can see @nridge 's reasoning about indicating copies by saying pass by 
> value vs ref, which unfortunately doesn't align with C++ semantics directly 
> (as what we have here is a prvalue, and it is indeed passed by value, without 
> any copies to the callee).
> 
> it isn't very obvious anywhere but the main functionality we wanted to 
> provide to the developer was help them figure out if a function call can 
> mutate a parameter they were passing in, therefore it didn't prioritise 
> literals at all. we probably should've made better wording choices in the UI 
> and talked about "immutability". hence from functionality point of view 
> calling this pass by `value` vs `const ref` doesn't make a huge difference 
> (but that's probably only in my mind and people are already using it to infer 
> other things like whether we're going to trigger copies).
> 
> so i'd actually leave this case as-is, and think about what we're actually 
> trying to provide by showing arg info on literals. as it's currently trying 
> to overload the meaning of `passby` and causing confusions. since the initial 
> intent was to just convey "immutability" one suggestion would be to just hide 
> the `passby` information for literals.
> otherwise from value categories point of view, these are always passed by 
> value, but this is going to create confusion for people that are using it to 
> infer "copies" and getting that right, while preserving the semantics around 
> "is this mutable" just complicates things.
> 
> best thing moving forward would probably be to just have two separate fields, 
> one indicating mutability and another indicating copies and not talking about 
> pass by type at all.
> 
> ---
> 
> sorry 

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

In D142014#4084263 , @nridge wrote:

> In D142014#4081922 , @v1nh1shungry 
> wrote:
>
>> I just came up with a case where the original implementation and this patch 
>> behave differently.
>>
>>   void foobar(const float &);
>>   int main() {
>> foobar(0);
>>^
>>   }
>>
>> Used to `Passed by value`, now it is `Passed by const reference`. I think 
>> the former one is apparently better.
>
> Why is "passed by value" better?
>
> My mental model here is that the value does not get copied; for a built-in 
> type that may not be much of a distinction, but if you imagine generalizing 
> to a class type constructed using a user-defined literal, it could be a 
> noncopyable (and in C++17, even non-moveable) type that can't be passed 
> around by value.

Ah, I think you're right. I was confused. This case seems good to me. I guess I 
was thinking about

  void foobar(const float &);
  int main() {
int a;
foobar(a);
   ^
  }

Used to be `Passed by value`, now it is `Passed by const reference`. I think 
it's kind of confusing because the local variable `a` isn't actually 
referenced, right?




Comment at: clang-tools-extra/clangd/Hover.cpp:994
   HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+  PassType.PassBy = getPassMode(PVD->getType());
+}

kadircet wrote:
> v1nh1shungry wrote:
> > kadircet wrote:
> > > sorry for showing up late to the party but instead of changing rest of 
> > > the code, can we apply this logic only when there are no implicit 
> > > casts/conversions between the arg and callexpr (i.e `N == `)?
> > To make sure I understand it correctly, do you mean I should give up any 
> > other code changes I made but keep this logic, and put this logic into the 
> > `N == ` branch?
> > 
> > Sorry if I misunderstood anything! Shame for not being a good reader :(
> > To make sure I understand it correctly, do you mean I should give up any 
> > other code changes I made but keep this logic, and put this logic into the 
> > N ==  branch?
> 
> Somewhat.
> 
> Basically this code had the assumption that if we don't see any 
> casts/conversions between the expression creating the argument and the 
> expression getting passed to the callexpr, it must be passed by reference, 
> and this was wrong. Even before the patch that added handling for literals.
> 
> The rest of the handling for casts/conversions/constructors in between have 
> been working so far and was constructed based on what each particular cast 
> does, not for specific cases hence they're easier (for the lack of a better 
> word) to reason about. Hence I'd rather keep them as is, especially the 
> changes in handling `MaterializeTemporaryExpr` don't sound right. I can see 
> the example you've at hand, but we shouldn't be producing "converted" results 
> for it anyway (the AST should have a NoOp implicit cast to `const int` and 
> then a `MaterializeTemporaryExpr`, which shouldn't generated any converted 
> signals with the existing code already).
> 
> Hence the my proposal is getting rid of the assumption around "if we don't 
> see any casts/conversions between the expression creating the argument and 
> the expression getting passed to the callexpr, it must be passed by 
> reference", and use the type information in `ParmVarDecl` directly when we 
> don't have any implicit nodes in between to infer `PassBy`.
> This should imply also getting rid of the special case for literals (`if 
> (isLiteral(E) && N->Parent == OuterNode.Parent)`).
> 
> Does that make sense?
Thanks for the detailed explanation! But before we go ahead here, what do you 
think about the new test case I'm talking about above? Do you agree with my 
conclusion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

I just came up with a case where the original implementation and this patch 
behave differently.

  void foobar(const float &);
  int main() {
foobar(0);
   ^
  }

Used to `Passed by value`, now it is `Passed by const reference`. I think the 
former one is apparently better.

This case can be fixed by getting the `PassType.PassBy = 
HoverInfo::PassType::Value;` back to the `ImplicitCastExpr` case, but hmm...It 
does make me hesitate to insist on the current code change. Maybe we should 
switch to @kadircet's idea if I understand him correctly. WDYT, @nridge, 
@tom-anders and @adamcz?

But the `isLiteral()` and `ImplicitCastExpr` cases in the original 
implementation bring enough mystery and risks to me. For me this is a hard 
decision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:994
   HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+  PassType.PassBy = getPassMode(PVD->getType());
+}

kadircet wrote:
> sorry for showing up late to the party but instead of changing rest of the 
> code, can we apply this logic only when there are no implicit 
> casts/conversions between the arg and callexpr (i.e `N == `)?
To make sure I understand it correctly, do you mean I should give up any other 
code changes I made but keep this logic, and put this logic into the `N == 
` branch?

Sorry if I misunderstood anything! Shame for not being a good reader :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

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

Thanks for the review! @tom-anders




Comment at: clang-tools-extra/clangd/Hover.cpp:1035
 } else if (const auto *MTE =
CastNode->ASTNode.get()) {
 } else { // Unknown implicit node, assume type conversion.

v1nh1shungry wrote:
> tom-anders wrote:
> > v1nh1shungry wrote:
> > > tom-anders wrote:
> > > > This branch is now empty, do we still need it? 
> > > I keep this branch because the original implementation doesn't want to 
> > > mark `Converted` in this branch. Yeah I can modify the following `else` 
> > > to `else if (const ...; !MTE)` (or something cleaner), but I think the 
> > > two are the same ugly :(
> > Ah I see, what about just adding `PassType.Converted = false` here (even 
> > though it has no effect, but just to make this more clear)? 
> Then how about just adding some comments? I don't have a strong opinion here 
> though.
Done. Sorry for the confusion here! Hope it is clear now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 491594.
v1nh1shungry added a comment.

land review's suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -934,6 +934,23 @@
  HI.CalleeArgInfo->Type = "const int &";
  HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, false};
}},
+  {
+  R"cpp(
+int add(int lhs, int rhs);
+int main() {
+  add(1 [[^+]] 2, 3);
+}
+)cpp",
+  [](HoverInfo ) {
+HI.Name = "expression";
+HI.Kind = index::SymbolKind::Unknown;
+HI.Type = "int";
+HI.Value = "3";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "lhs";
+HI.CalleeArgInfo->Type = "int";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, false};
+  }},
   {// Extra info for method call.
R"cpp(
   class C {
@@ -1673,8 +1690,8 @@
   }},
   {
   R"cpp(// Function definition via using declaration
-namespace ns { 
-  void foo(); 
+namespace ns {
+  void foo();
 }
 int main() {
   using ns::foo;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -952,6 +952,15 @@
   }
 }
 
+HoverInfo::PassType::PassMode getPassMode(QualType ParmType) {
+  if (ParmType->isReferenceType()) {
+if (ParmType->getPointeeType().isConstQualified())
+  return HoverInfo::PassType::ConstRef;
+return HoverInfo::PassType::Ref;
+  }
+  return HoverInfo::PassType::Value;
+}
+
 // If N is passed as argument to a function, fill HI.CalleeArgInfo with
 // information about that argument.
 void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo ,
@@ -972,14 +981,18 @@
   if (!FD || FD->isOverloadedOperator() || FD->isVariadic())
 return;
 
+  HoverInfo::PassType PassType;
+
   // Find argument index for N.
   for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
 if (CE->getArg(I) != OuterNode.ASTNode.get())
   continue;
 
 // Extract matching argument from function declaration.
-if (const ParmVarDecl *PVD = FD->getParamDecl(I))
+if (const ParmVarDecl *PVD = FD->getParamDecl(I)) {
   HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+  PassType.PassBy = getPassMode(PVD->getType());
+}
 break;
   }
   if (!HI.CalleeArgInfo)
@@ -988,16 +1001,6 @@
   // If we found a matching argument, also figure out if it's a
   // [const-]reference. For this we need to walk up the AST from the arg itself
   // to CallExpr and check all implicit casts, constructor calls, etc.
-  HoverInfo::PassType PassType;
-  if (const auto *E = N->ASTNode.get()) {
-if (E->getType().isConstQualified())
-  PassType.PassBy = HoverInfo::PassType::ConstRef;
-
-// No implicit node, literal passed by value
-if (isLiteral(E) && N->Parent == OuterNode.Parent)
-  PassType.PassBy = HoverInfo::PassType::Value;
-  }
-
   for (auto *CastNode = N->Parent;
CastNode != OuterNode.Parent && !PassType.Converted;
CastNode = CastNode->Parent) {
@@ -1006,22 +1009,13 @@
   case CK_NoOp:
   case CK_DerivedToBase:
   case CK_UncheckedDerivedToBase:
-// If it was a reference before, it's still a reference.
-if (PassType.PassBy != HoverInfo::PassType::Value)
-  PassType.PassBy = ImplicitCast->getType().isConstQualified()
-? HoverInfo::PassType::ConstRef
-: HoverInfo::PassType::Ref;
-break;
   case CK_LValueToRValue:
   case CK_ArrayToPointerDecay:
   case CK_FunctionToPointerDecay:
   case CK_NullToPointer:
   case CK_NullToMemberPointer:
-// No longer a reference, but we do not show this as type conversion.
-PassType.PassBy = HoverInfo::PassType::Value;
 break;
   default:
-PassType.PassBy = HoverInfo::PassType::Value;
 PassType.Converted = true;
 break;
   }
@@ -1029,16 +1023,24 @@
CastNode->ASTNode.get()) {
   // We want to be smart about copy constructors. They should not show up as
   // type conversion, but instead as passing by value.
-  if (CtorCall->getConstructor()->isCopyConstructor())
+  const CXXConstructorDecl *CD = CtorCall->getConstructor();
+  if (CD->isCopyConstructor()) {
 

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry marked an inline comment as done.
v1nh1shungry added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:1035
 } else if (const auto *MTE =
CastNode->ASTNode.get()) {
 } else { // Unknown implicit node, assume type conversion.

tom-anders wrote:
> v1nh1shungry wrote:
> > tom-anders wrote:
> > > This branch is now empty, do we still need it? 
> > I keep this branch because the original implementation doesn't want to mark 
> > `Converted` in this branch. Yeah I can modify the following `else` to `else 
> > if (const ...; !MTE)` (or something cleaner), but I think the two are the 
> > same ugly :(
> Ah I see, what about just adding `PassType.Converted = false` here (even 
> though it has no effect, but just to make this more clear)? 
Then how about just adding some comments? I don't have a strong opinion here 
though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:1035
 } else if (const auto *MTE =
CastNode->ASTNode.get()) {
 } else { // Unknown implicit node, assume type conversion.

tom-anders wrote:
> This branch is now empty, do we still need it? 
I keep this branch because the original implementation doesn't want to mark 
`Converted` in this branch. Yeah I can modify the following `else` to `else if 
(const ...; !MTE)` (or something cleaner), but I think the two are the same 
ugly :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 491440.
v1nh1shungry added a comment.

`==` to `>=`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -934,6 +934,23 @@
  HI.CalleeArgInfo->Type = "const int &";
  HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, false};
}},
+  {
+  R"cpp(
+int add(int lhs, int rhs);
+int main() {
+  add(1 [[^+]] 2, 3);
+}
+)cpp",
+  [](HoverInfo ) {
+HI.Name = "expression";
+HI.Kind = index::SymbolKind::Unknown;
+HI.Type = "int";
+HI.Value = "3";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "lhs";
+HI.CalleeArgInfo->Type = "int";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, false};
+  }},
   {// Extra info for method call.
R"cpp(
   class C {
@@ -1673,8 +1690,8 @@
   }},
   {
   R"cpp(// Function definition via using declaration
-namespace ns { 
-  void foo(); 
+namespace ns {
+  void foo();
 }
 int main() {
   using ns::foo;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -952,6 +952,15 @@
   }
 }
 
+HoverInfo::PassType::PassMode getPassMode(QualType ParmType) {
+  if (ParmType->isReferenceType()) {
+if (ParmType->getPointeeType().isConstQualified())
+  return HoverInfo::PassType::ConstRef;
+return HoverInfo::PassType::Ref;
+  }
+  return HoverInfo::PassType::Value;
+}
+
 // If N is passed as argument to a function, fill HI.CalleeArgInfo with
 // information about that argument.
 void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo ,
@@ -972,14 +981,18 @@
   if (!FD || FD->isOverloadedOperator() || FD->isVariadic())
 return;
 
+  HoverInfo::PassType PassType;
+
   // Find argument index for N.
   for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
 if (CE->getArg(I) != OuterNode.ASTNode.get())
   continue;
 
 // Extract matching argument from function declaration.
-if (const ParmVarDecl *PVD = FD->getParamDecl(I))
+if (const ParmVarDecl *PVD = FD->getParamDecl(I)) {
   HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+  PassType.PassBy = getPassMode(PVD->getType());
+}
 break;
   }
   if (!HI.CalleeArgInfo)
@@ -988,16 +1001,6 @@
   // If we found a matching argument, also figure out if it's a
   // [const-]reference. For this we need to walk up the AST from the arg itself
   // to CallExpr and check all implicit casts, constructor calls, etc.
-  HoverInfo::PassType PassType;
-  if (const auto *E = N->ASTNode.get()) {
-if (E->getType().isConstQualified())
-  PassType.PassBy = HoverInfo::PassType::ConstRef;
-
-// No implicit node, literal passed by value
-if (isLiteral(E) && N->Parent == OuterNode.Parent)
-  PassType.PassBy = HoverInfo::PassType::Value;
-  }
-
   for (auto *CastNode = N->Parent;
CastNode != OuterNode.Parent && !PassType.Converted;
CastNode = CastNode->Parent) {
@@ -1006,22 +1009,13 @@
   case CK_NoOp:
   case CK_DerivedToBase:
   case CK_UncheckedDerivedToBase:
-// If it was a reference before, it's still a reference.
-if (PassType.PassBy != HoverInfo::PassType::Value)
-  PassType.PassBy = ImplicitCast->getType().isConstQualified()
-? HoverInfo::PassType::ConstRef
-: HoverInfo::PassType::Ref;
-break;
   case CK_LValueToRValue:
   case CK_ArrayToPointerDecay:
   case CK_FunctionToPointerDecay:
   case CK_NullToPointer:
   case CK_NullToMemberPointer:
-// No longer a reference, but we do not show this as type conversion.
-PassType.PassBy = HoverInfo::PassType::Value;
 break;
   default:
-PassType.PassBy = HoverInfo::PassType::Value;
 PassType.Converted = true;
 break;
   }
@@ -1029,16 +1023,17 @@
CastNode->ASTNode.get()) {
   // We want to be smart about copy constructors. They should not show up as
   // type conversion, but instead as passing by value.
-  if (CtorCall->getConstructor()->isCopyConstructor())
+  const CXXConstructorDecl *CD = CtorCall->getConstructor();
+  if (CD->isCopyConstructor()) {
 

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry marked 2 inline comments as done.
v1nh1shungry added a comment.

Thanks for the review! @nridge


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 491287.
v1nh1shungry added a comment.

land comment's suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142014

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -934,6 +934,23 @@
  HI.CalleeArgInfo->Type = "const int &";
  HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, false};
}},
+  {
+  R"cpp(
+int add(int lhs, int rhs);
+int main() {
+  add(1 [[^+]] 2, 3);
+}
+)cpp",
+  [](HoverInfo ) {
+HI.Name = "expression";
+HI.Kind = index::SymbolKind::Unknown;
+HI.Type = "int";
+HI.Value = "3";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "lhs";
+HI.CalleeArgInfo->Type = "int";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, false};
+  }},
   {// Extra info for method call.
R"cpp(
   class C {
@@ -1673,8 +1690,8 @@
   }},
   {
   R"cpp(// Function definition via using declaration
-namespace ns { 
-  void foo(); 
+namespace ns {
+  void foo();
 }
 int main() {
   using ns::foo;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -952,6 +952,15 @@
   }
 }
 
+HoverInfo::PassType::PassMode getPassMode(QualType ParmType) {
+  if (ParmType->isReferenceType()) {
+if (ParmType->getPointeeType().isConstQualified())
+  return HoverInfo::PassType::ConstRef;
+return HoverInfo::PassType::Ref;
+  }
+  return HoverInfo::PassType::Value;
+}
+
 // If N is passed as argument to a function, fill HI.CalleeArgInfo with
 // information about that argument.
 void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo ,
@@ -972,14 +981,18 @@
   if (!FD || FD->isOverloadedOperator() || FD->isVariadic())
 return;
 
+  HoverInfo::PassType PassType;
+
   // Find argument index for N.
   for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
 if (CE->getArg(I) != OuterNode.ASTNode.get())
   continue;
 
 // Extract matching argument from function declaration.
-if (const ParmVarDecl *PVD = FD->getParamDecl(I))
+if (const ParmVarDecl *PVD = FD->getParamDecl(I)) {
   HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+  PassType.PassBy = getPassMode(PVD->getType());
+}
 break;
   }
   if (!HI.CalleeArgInfo)
@@ -988,16 +1001,6 @@
   // If we found a matching argument, also figure out if it's a
   // [const-]reference. For this we need to walk up the AST from the arg itself
   // to CallExpr and check all implicit casts, constructor calls, etc.
-  HoverInfo::PassType PassType;
-  if (const auto *E = N->ASTNode.get()) {
-if (E->getType().isConstQualified())
-  PassType.PassBy = HoverInfo::PassType::ConstRef;
-
-// No implicit node, literal passed by value
-if (isLiteral(E) && N->Parent == OuterNode.Parent)
-  PassType.PassBy = HoverInfo::PassType::Value;
-  }
-
   for (auto *CastNode = N->Parent;
CastNode != OuterNode.Parent && !PassType.Converted;
CastNode = CastNode->Parent) {
@@ -1006,22 +1009,13 @@
   case CK_NoOp:
   case CK_DerivedToBase:
   case CK_UncheckedDerivedToBase:
-// If it was a reference before, it's still a reference.
-if (PassType.PassBy != HoverInfo::PassType::Value)
-  PassType.PassBy = ImplicitCast->getType().isConstQualified()
-? HoverInfo::PassType::ConstRef
-: HoverInfo::PassType::Ref;
-break;
   case CK_LValueToRValue:
   case CK_ArrayToPointerDecay:
   case CK_FunctionToPointerDecay:
   case CK_NullToPointer:
   case CK_NullToMemberPointer:
-// No longer a reference, but we do not show this as type conversion.
-PassType.PassBy = HoverInfo::PassType::Value;
 break;
   default:
-PassType.PassBy = HoverInfo::PassType::Value;
 PassType.Converted = true;
 break;
   }
@@ -1029,16 +1023,17 @@
CastNode->ASTNode.get()) {
   // We want to be smart about copy constructors. They should not show up as
   // type conversion, but instead as passing by value.
-  if (CtorCall->getConstructor()->isCopyConstructor())
+  const CXXConstructorDecl *CD = CtorCall->getConstructor();
+  if (CD->isCopyConstructor()) {
 

[PATCH] D141787: [clang-tidy] fix a false positive of `modernize-concat-nested-namespaces`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Ping? @njames93




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp:173
+namespace n48 {
+// CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be 
concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n47::n48

v1nh1shungry wrote:
> njames93 wrote:
> > Is `CHECK-MESSAGES-DAG` needed here, why does it fail if this is omitted?
> Hmm, doesn't `CHECK-MESSAGES-DAG` means to check whether there is a message? 
> (I referred to the tests above.)
> 
> If it's other cases, maybe it's due to 
> https://github.com/llvm/llvm-project/issues/60051.
> 
> I'm not sure I understand what you mean, sorry!
> Hmm, doesn't `CHECK-MESSAGES-DAG` means to check whether there is a message? 
> (I referred to the tests above.)
> 
> If it's other cases, maybe it's due to 
> https://github.com/llvm/llvm-project/issues/60051.
> 
> I'm not sure I understand what you mean, sorry!

I think I understand now. You mean the whole test doesn't fail even without 
this `CHECK-MESSAGES-DAG`, right?

If so, this `CHECK-MESSAGES-DAG` is needed. I think the reason that the test 
doesn't fail without it is https://github.com/llvm/llvm-project/issues/60051. 
The check isn't applied correctly before I make any code changes, you can have 
a play with https://godbolt.org/z/nc34shM8W. But I think the problem isn't 
related to the check itself because when I apply this check through clangd's 
code action it produces correct codes as I mentioned in the GitHub issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141787

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

[PATCH] D140838: [clang] fix crash on generic lambda with lambda in decltype

[Vincent Hong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGea4fd668c2cd: [clang] fix crash on generic lambda with 
lambda in decltype (authored by v1nh1shungry).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140838

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaCXX/lambda-unevaluated.cpp


Index: clang/test/SemaCXX/lambda-unevaluated.cpp
===
--- clang/test/SemaCXX/lambda-unevaluated.cpp
+++ clang/test/SemaCXX/lambda-unevaluated.cpp
@@ -142,3 +142,7 @@
 using d = decltype(sizeof([] static { return 0; }));
 
 }
+
+namespace lambda_in_trailing_decltype {
+auto x = ([](auto) -> decltype([] {}()) {}(0), 2);
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1231,7 +1231,7 @@
 
   // We recreated a local declaration, but not by instantiating it. There
   // may be pending dependent diagnostics to produce.
-  if (auto *DC = dyn_cast(Old))
+  if (auto *DC = dyn_cast(Old); DC && 
DC->isDependentContext())
 SemaRef.PerformDependentDiagnostics(DC, TemplateArgs);
 }
 


Index: clang/test/SemaCXX/lambda-unevaluated.cpp
===
--- clang/test/SemaCXX/lambda-unevaluated.cpp
+++ clang/test/SemaCXX/lambda-unevaluated.cpp
@@ -142,3 +142,7 @@
 using d = decltype(sizeof([] static { return 0; }));
 
 }
+
+namespace lambda_in_trailing_decltype {
+auto x = ([](auto) -> decltype([] {}()) {}(0), 2);
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1231,7 +1231,7 @@
 
   // We recreated a local declaration, but not by instantiating it. There
   // may be pending dependent diagnostics to produce.
-  if (auto *DC = dyn_cast(Old))
+  if (auto *DC = dyn_cast(Old); DC && DC->isDependentContext())
 SemaRef.PerformDependentDiagnostics(DC, TemplateArgs);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

In D139705#4067774 , @lattner wrote:

> Got it this time, sorry for the confusion!

Confirmed that I received the invitation. Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

In D139705#4067370 , @lattner wrote:

> I'm pretty sure I'm on top of commit access requests now, plz let me know if 
> I missed you or something!  Could be spam filter or who knows what

I sent an email from  to 
 on last Friday, and I've received no reply so far. I've 
checked the email address many times so it was not likely to be sent to the 
wrong person.

I can resend the email. Or maybe we can deal with it here? If so, my GitHub 
username is v1nh1shungry (https://github.com/v1nh1shungry).

Thank you so much!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Sorry to disturb you again! It's been a week since I sent an email to Chris, 
and I have received no reply. Maybe my request was refused. It'd be great to 
land this and https://reviews.llvm.org/D140838 before the 16 release, right? If 
so, could you please help me commit them? I'd like "v1nh1shungry 
". Thank you so much!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 490525.
v1nh1shungry added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/DeclTemplate.h
  clang/test/FixIt/fixit-const-var-init.cpp


Index: clang/test/FixIt/fixit-const-var-init.cpp
===
--- /dev/null
+++ clang/test/FixIt/fixit-const-var-init.cpp
@@ -0,0 +1,28 @@
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++14 %s 
2>&1 | FileCheck %s
+
+const int a; // expected-error {{default initialization of an object of const 
type}}
+// CHECK: fix-it:"{{.*}}":{3:12-3:12}:" = 0"
+
+template  const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{6:36-6:36}:" = 0"
+
+template  const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{9:39-9:39}:" = 0"
+
+template <> const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{12:36-12:36}:" = 0"
+
+constexpr float c; // expected-error {{must be initialized by a constant 
expression}}
+// CHECK: fix-it:"{{.*}}":{15:18-15:18}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{18:42-18:42}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{21:45-21:45}:" = 0.0"
+
+template <> constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{24:42-24:42}:" = 0.0"
+
+void (* const func)(int, int); // expected-error {{default initialization of 
an object of const type}}
+// CHECK: fix-it:"{{.*}}":{27:30-27:30}:" = nullptr"
Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -2926,6 +2926,14 @@
 return ExplicitInfo ? ExplicitInfo->TemplateKeywordLoc : SourceLocation();
   }
 
+  SourceRange getSourceRange() const override LLVM_READONLY {
+if (isExplicitSpecialization()) {
+  if (const ASTTemplateArgumentListInfo *Info = getTemplateArgsInfo())
+return SourceRange(getOuterLocStart(), Info->getRAngleLoc());
+}
+return VarDecl::getSourceRange();
+  }
+
   void Profile(llvm::FoldingSetNodeID ) const {
 Profile(ID, TemplateArgs->asArray(), getASTContext());
   }
@@ -3083,6 +3091,14 @@
 return First->InstantiatedFromMember.setInt(true);
   }
 
+  SourceRange getSourceRange() const override LLVM_READONLY {
+if (isExplicitSpecialization()) {
+  if (const ASTTemplateArgumentListInfo *Info = getTemplateArgsAsWritten())
+return SourceRange(getOuterLocStart(), Info->getRAngleLoc());
+}
+return VarDecl::getSourceRange();
+  }
+
   void Profile(llvm::FoldingSetNodeID ) const {
 Profile(ID, getTemplateArgs().asArray(), getTemplateParameters(),
 getASTContext());
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -350,6 +350,8 @@
   This fixes `Issue 59765 `_
 - Reject in-class defaulting of previosly declared comparison operators. Fixes
   `Issue 51227 `_.
+- Fix the bug of inserting the ``ZeroInitializationFixit`` before the template
+  argument list of ``VarTemplateSpecializationDecl``.
 
 Improvements to Clang's diagnostics
 ^^^


Index: clang/test/FixIt/fixit-const-var-init.cpp
===
--- /dev/null
+++ clang/test/FixIt/fixit-const-var-init.cpp
@@ -0,0 +1,28 @@
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++14 %s 2>&1 | FileCheck %s
+
+const int a; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{3:12-3:12}:" = 0"
+
+template  const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{6:36-6:36}:" = 0"
+
+template  const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{9:39-9:39}:" = 0"
+
+template <> const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{12:36-12:36}:" = 0"
+
+constexpr float c; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{15:18-15:18}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be initialized by a constant expression}}
+// CHECK: 

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added reviewers: tom-anders, nridge.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

  void foobar(int);
  int main() {
foobar(1 + 2);
 ^
  }

Currently the CalleeArgInfo will be "Passed by reference", which should
be "Passed by value".

Fixes https://github.com/clangd/clangd/issues/1467


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142014

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -934,6 +934,23 @@
  HI.CalleeArgInfo->Type = "const int &";
  HI.CallPassType = HoverInfo::PassType{PassMode::ConstRef, false};
}},
+  {
+  R"cpp(
+int add(int lhs, int rhs);
+int main() {
+  add(1 [[^+]] 2, 3);
+}
+)cpp",
+  [](HoverInfo ) {
+HI.Name = "expression";
+HI.Kind = index::SymbolKind::Unknown;
+HI.Type = "int";
+HI.Value = "3";
+HI.CalleeArgInfo.emplace();
+HI.CalleeArgInfo->Name = "lhs";
+HI.CalleeArgInfo->Type = "int";
+HI.CallPassType = HoverInfo::PassType{PassMode::Value, false};
+  }},
   {// Extra info for method call.
R"cpp(
   class C {
@@ -1673,8 +1690,8 @@
   }},
   {
   R"cpp(// Function definition via using declaration
-namespace ns { 
-  void foo(); 
+namespace ns {
+  void foo();
 }
 int main() {
   using ns::foo;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -952,6 +952,15 @@
   }
 }
 
+HoverInfo::PassType::PassMode getPassMode(QualType QT) {
+  if (QT->isReferenceType()) {
+if (QT->getPointeeType().isConstQualified())
+  return HoverInfo::PassType::ConstRef;
+return HoverInfo::PassType::Ref;
+  }
+  return HoverInfo::PassType::Value;
+}
+
 // If N is passed as argument to a function, fill HI.CalleeArgInfo with
 // information about that argument.
 void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo ,
@@ -972,14 +981,18 @@
   if (!FD || FD->isOverloadedOperator() || FD->isVariadic())
 return;
 
+  QualType ParmType;
+
   // Find argument index for N.
   for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
 if (CE->getArg(I) != OuterNode.ASTNode.get())
   continue;
 
 // Extract matching argument from function declaration.
-if (const ParmVarDecl *PVD = FD->getParamDecl(I))
+if (const ParmVarDecl *PVD = FD->getParamDecl(I)) {
   HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+  ParmType = PVD->getType();
+}
 break;
   }
   if (!HI.CalleeArgInfo)
@@ -989,15 +1002,7 @@
   // [const-]reference. For this we need to walk up the AST from the arg itself
   // to CallExpr and check all implicit casts, constructor calls, etc.
   HoverInfo::PassType PassType;
-  if (const auto *E = N->ASTNode.get()) {
-if (E->getType().isConstQualified())
-  PassType.PassBy = HoverInfo::PassType::ConstRef;
-
-// No implicit node, literal passed by value
-if (isLiteral(E) && N->Parent == OuterNode.Parent)
-  PassType.PassBy = HoverInfo::PassType::Value;
-  }
-
+  PassType.PassBy = getPassMode(ParmType);
   for (auto *CastNode = N->Parent;
CastNode != OuterNode.Parent && !PassType.Converted;
CastNode = CastNode->Parent) {
@@ -1006,22 +1011,13 @@
   case CK_NoOp:
   case CK_DerivedToBase:
   case CK_UncheckedDerivedToBase:
-// If it was a reference before, it's still a reference.
-if (PassType.PassBy != HoverInfo::PassType::Value)
-  PassType.PassBy = ImplicitCast->getType().isConstQualified()
-? HoverInfo::PassType::ConstRef
-: HoverInfo::PassType::Ref;
-break;
   case CK_LValueToRValue:
   case CK_ArrayToPointerDecay:
   case CK_FunctionToPointerDecay:
   case CK_NullToPointer:
   case CK_NullToMemberPointer:
-// No longer a reference, but we do not show this as type conversion.
-PassType.PassBy = HoverInfo::PassType::Value;
 break;
   default:
-PassType.PassBy = HoverInfo::PassType::Value;
 PassType.Converted = true;
 break;
   }
@@ -1029,16 +1025,16 @@
CastNode->ASTNode.get()) {
   // We want to be 

[PATCH] D141838: [clang-tidy] fix a false positive of `cppcoreguidelines-avoid-non-const-global-variables`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added reviewers: carlosgalvezp, gribozavr2.
Herald added subscribers: shchenz, kbarton, xazax.hun, nemanjai.
Herald added a reviewer: njames93.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Currently this checker will report the non-const static
private/protected member of a class/struct.

Fixes https://github.com/llvm/llvm-project/issues/57919


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141838

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
@@ -228,6 +228,20 @@
 template 
 constexpr T templateVariable = T(0L);
 
+// CHECKING FOR NON-CONST STATIC PRIVATE/PROTECTED CLASS MEMBERS //
+class Foo {
+  static int nonConstStaticPrivateMember;
+protected:
+  static int nonConstStaticProtectedMember;
+};
+
+struct Bar {
+private:
+  static int nonConstStaticPrivateMember;
+protected:
+  static int nonConstStaticProtectedMember;
+};
+
 // CHECKING AGAINST FALSE POSITIVES INSIDE FUNCTION SCOPE /
 int main() {
   for (int i = 0; i < 3; ++i) {
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
@@ -24,6 +24,7 @@
   hasGlobalStorage(),
   unless(anyOf(
   isLocalVarDecl(), isConstexpr(), hasType(isConstQualified()),
+  isPrivate(), isProtected(),
   hasType(referenceType(); // References can't be changed, only the
// data they reference can be changed.
 


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
@@ -228,6 +228,20 @@
 template 
 constexpr T templateVariable = T(0L);
 
+// CHECKING FOR NON-CONST STATIC PRIVATE/PROTECTED CLASS MEMBERS //
+class Foo {
+  static int nonConstStaticPrivateMember;
+protected:
+  static int nonConstStaticProtectedMember;
+};
+
+struct Bar {
+private:
+  static int nonConstStaticPrivateMember;
+protected:
+  static int nonConstStaticProtectedMember;
+};
+
 // CHECKING AGAINST FALSE POSITIVES INSIDE FUNCTION SCOPE /
 int main() {
   for (int i = 0; i < 3; ++i) {
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
@@ -24,6 +24,7 @@
   hasGlobalStorage(),
   unless(anyOf(
   isLocalVarDecl(), isConstexpr(), hasType(isConstQualified()),
+  isPrivate(), isProtected(),
   hasType(referenceType(); // References can't be changed, only the
// data they reference can be changed.
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141787: [clang-tidy] fix a false positive of `modernize-concat-nested-namespaces`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp:173
+namespace n48 {
+// CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be 
concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n47::n48

njames93 wrote:
> Is `CHECK-MESSAGES-DAG` needed here, why does it fail if this is omitted?
Hmm, doesn't `CHECK-MESSAGES-DAG` means to check whether there is a message? (I 
referred to the tests above.)

If it's other cases, maybe it's due to 
https://github.com/llvm/llvm-project/issues/60051.

I'm not sure I understand what you mean, sorry!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141787

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

[PATCH] D141787: [clang-tidy] fix a false positive of `modernize-concat-nested-namespaces`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

In D141787#4054514 , @v1nh1shungry 
wrote:

> This patch doesn't handle situations like:
>
>   namespace a {
>   namespace b {
>   #define FOO
>   namespace c {
>   }
>   }
>   }

It did. I must be mad :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141787

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

[PATCH] D141787: [clang-tidy] fix a false positive of `modernize-concat-nested-namespaces`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 489350.
v1nh1shungry added a comment.

add a test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141787

Files:
  clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
@@ -156,6 +156,29 @@
 } // namespace n41
 // CHECK-FIXES: }
 
+namespace n43 {
+#define FOO
+namespace n44 {
+} // namespace n44
+} // namespace n43
+
+namespace n45 {
+namespace n46 {
+} // namespace n46
+#define BAR
+} // namespace n45
+
+namespace n47 {
+namespace n48 {
+// CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n47::n48
+#define FOOBAR
+namespace n49 {
+} // namespace n49
+} // namespace n48
+} // namespace n47
+// CHECK-FIXES: }
+
 int main() {
   n26::n27::n28::n29::n30::t();
 #ifdef IEXIST
Index: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
@@ -24,13 +24,37 @@
   return ND.isAnonymousNamespace() || ND.isInlineNamespace();
 }
 
-static bool singleNamedNamespaceChild(const NamespaceDecl ) {
+static bool hasPPDirective(SourceRange Range, const SourceManager ,
+   const LangOptions ) {
+  CharSourceRange CharRange =
+  Lexer::makeFileCharRange({Range, true}, SM, LangOpts);
+  StringRef BetweenText = Lexer::getSourceText(CharRange, SM, LangOpts);
+  std::string Buffer{BetweenText};
+  Lexer Lex(Range.getBegin(), LangOpts, Buffer.c_str(), Buffer.c_str(),
+Buffer.c_str() + Buffer.size());
+  Token Tok;
+  while (!Lex.LexFromRawLexer(Tok)) {
+if (Tok.getKind() == tok::hash)
+  return true;
+  }
+  return false;
+}
+
+static bool singleNamedNamespaceChild(const NamespaceDecl ,
+  const SourceManager ,
+  const LangOptions ) {
   NamespaceDecl::decl_range Decls = ND.decls();
   if (std::distance(Decls.begin(), Decls.end()) != 1)
 return false;
 
   const auto *ChildNamespace = dyn_cast(*Decls.begin());
-  return ChildNamespace && !anonymousOrInlineNamespace(*ChildNamespace);
+  if (!ChildNamespace || anonymousOrInlineNamespace(*ChildNamespace))
+return false;
+
+  return !hasPPDirective({ND.getBeginLoc(), ChildNamespace->getBeginLoc()}, SM,
+ LangOpts) &&
+ !hasPPDirective({ChildNamespace->getRBraceLoc(), ND.getRBraceLoc()},
+ SM, LangOpts);
 }
 
 static bool alreadyConcatenated(std::size_t NumCandidates,
@@ -76,6 +100,7 @@
 const ast_matchers::MatchFinder::MatchResult ) {
   const NamespaceDecl  = *Result.Nodes.getNodeAs("namespace");
   const SourceManager  = *Result.SourceManager;
+  const LangOptions  = getLangOpts();
 
   if (!locationsInSameFile(Sources, ND.getBeginLoc(), ND.getRBraceLoc()))
 return;
@@ -85,7 +110,7 @@
 
   Namespaces.push_back();
 
-  if (singleNamedNamespaceChild(ND))
+  if (singleNamedNamespaceChild(ND, Sources, LangOpts))
 return;
 
   SourceRange FrontReplacement(Namespaces.front()->getBeginLoc(),
@@ -94,7 +119,7 @@
   Namespaces.front()->getRBraceLoc());
 
   if (!alreadyConcatenated(Namespaces.size(), FrontReplacement, Sources,
-   getLangOpts()))
+   LangOpts))
 reportDiagnostic(FrontReplacement, BackReplacement);
 
   Namespaces.clear();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141787: [clang-tidy] fix a false positive of `modernize-concat-nested-namespaces`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry planned changes to this revision.
v1nh1shungry added a comment.

This patch doesn't handle situations like:

  namespace a {
  namespace b {
  #define FOO
  namespace c {
  }
  }
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141787

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

[PATCH] D141787: [clang-tidy] fix a false positive of `modernize-concat-nested-namespaces`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added reviewers: carlosgalvezp, Eugene.Zelenko.
Herald added a subscriber: xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Currently this check will complain when there're preprocessor directives
like macro definitions between the namespaces, e.g.

  namespace a { // warns, but it shouldn't
   #define FOO
  namespace b {
  } // namespace b
  } // namespace a

Fixes https://github.com/llvm/llvm-project/issues/60035 partly


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141787

Files:
  clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
@@ -156,6 +156,18 @@
 } // namespace n41
 // CHECK-FIXES: }
 
+namespace n43 {
+#define FOO
+namespace n44 {
+} // namespace n44
+} // namespace n43
+
+namespace n45 {
+namespace n46 {
+} // namespace n46
+#define BAR
+} // namespace n45
+
 int main() {
   n26::n27::n28::n29::n30::t();
 #ifdef IEXIST
Index: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
@@ -24,13 +24,37 @@
   return ND.isAnonymousNamespace() || ND.isInlineNamespace();
 }
 
-static bool singleNamedNamespaceChild(const NamespaceDecl ) {
+static bool hasPPDirective(SourceRange Range, const SourceManager ,
+   const LangOptions ) {
+  CharSourceRange CharRange =
+  Lexer::makeFileCharRange({Range, true}, SM, LangOpts);
+  StringRef BetweenText = Lexer::getSourceText(CharRange, SM, LangOpts);
+  std::string Buffer{BetweenText};
+  Lexer Lex(Range.getBegin(), LangOpts, Buffer.c_str(), Buffer.c_str(),
+Buffer.c_str() + Buffer.size());
+  Token Tok;
+  while (!Lex.LexFromRawLexer(Tok)) {
+if (Tok.getKind() == tok::hash)
+  return true;
+  }
+  return false;
+}
+
+static bool singleNamedNamespaceChild(const NamespaceDecl ,
+  const SourceManager ,
+  const LangOptions ) {
   NamespaceDecl::decl_range Decls = ND.decls();
   if (std::distance(Decls.begin(), Decls.end()) != 1)
 return false;
 
   const auto *ChildNamespace = dyn_cast(*Decls.begin());
-  return ChildNamespace && !anonymousOrInlineNamespace(*ChildNamespace);
+  if (!ChildNamespace || anonymousOrInlineNamespace(*ChildNamespace))
+return false;
+
+  return !hasPPDirective({ND.getBeginLoc(), ChildNamespace->getBeginLoc()}, SM,
+ LangOpts) &&
+ !hasPPDirective({ChildNamespace->getRBraceLoc(), ND.getRBraceLoc()},
+ SM, LangOpts);
 }
 
 static bool alreadyConcatenated(std::size_t NumCandidates,
@@ -76,6 +100,7 @@
 const ast_matchers::MatchFinder::MatchResult ) {
   const NamespaceDecl  = 
*Result.Nodes.getNodeAs("namespace");
   const SourceManager  = *Result.SourceManager;
+  const LangOptions  = getLangOpts();
 
   if (!locationsInSameFile(Sources, ND.getBeginLoc(), ND.getRBraceLoc()))
 return;
@@ -85,7 +110,7 @@
 
   Namespaces.push_back();
 
-  if (singleNamedNamespaceChild(ND))
+  if (singleNamedNamespaceChild(ND, Sources, LangOpts))
 return;
 
   SourceRange FrontReplacement(Namespaces.front()->getBeginLoc(),
@@ -94,7 +119,7 @@
   Namespaces.front()->getRBraceLoc());
 
   if (!alreadyConcatenated(Namespaces.size(), FrontReplacement, Sources,
-   getLangOpts()))
+   LangOpts))
 reportDiagnostic(FrontReplacement, BackReplacement);
 
   Namespaces.clear();


Index: clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
@@ -156,6 +156,18 @@
 } // namespace n41
 // CHECK-FIXES: }
 
+namespace n43 {
+#define FOO
+namespace n44 {
+} // namespace n44
+} // namespace n43
+
+namespace n45 {
+namespace n46 {
+} // namespace n46
+#define BAR
+} // namespace n45
+
 int main() {
   n26::n27::n28::n29::n30::t();
 #ifdef IEXIST
Index: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp

[PATCH] D140838: [clang] fix crash on generic lambda with lambda in decltype

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thanks for reviewing!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140838

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

> I'm happy to, what name and email address would you like used for patch 
> attribution?

I'd like "v1nh1shungry" and "v1nh1shun...@outlook.com". Thank you!

> Alternatively, it seems that you've had a few patches accepted in the 
> project, so you could apply for commit access yourself if you'd like: 
> https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Cool! I'll give it a try. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

If this patch is okay to land, could you please help me commit it? Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thank you for reviewing, @cjdb and @denik! If this patch is okay to land, could 
you please help me commit it? Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141107

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

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry marked 2 inline comments as done.
v1nh1shungry added a comment.

Thank you for reviewing and giving guidance!

> Do you have commit access?

I don't. If this patch is okay to land, could you please help me commit it? 
Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

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

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 488297.
v1nh1shungry added a comment.

modify the comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check-lines.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/test/code-action-request.test
  clang-tools-extra/clangd/test/request-reply.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -129,7 +129,7 @@
 "tweaks/DumpASTTests.cpp",
 "tweaks/DumpRecordLayoutTests.cpp",
 "tweaks/DumpSymbolTests.cpp",
-"tweaks/ExpandAutoTypeTests.cpp",
+"tweaks/ExpandDeducedTypeTests.cpp",
 "tweaks/ExpandMacroTests.cpp",
 "tweaks/ExtractFunctionTests.cpp",
 "tweaks/ExtractVariableTests.cpp",
Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -18,7 +18,7 @@
 "DefineInline.cpp",
 "DefineOutline.cpp",
 "DumpAST.cpp",
-"ExpandAutoType.cpp",
+"ExpandDeducedType.cpp",
 "ExpandMacro.cpp",
 "ExtractFunction.cpp",
 "ExtractVariable.cpp",
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -1611,7 +1611,7 @@
 clang-tools-extra/clangd/unittests/tweaks/DumpASTTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpRecordLayoutTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpSymbolTests.cpp
-clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExpandMacroTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -25,9 +25,9 @@
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
-// TWEAK_TEST(ExpandAutoType);
+// TWEAK_TEST(ExpandDeducedType);
 //
-// TEST_F(ExpandAutoTypeTest, ShortensTypes) {
+// TEST_F(ExpandDeducedTypeTest, ShortensTypes) {
 //   Header = R"cpp(
 // namespace foo { template class X{}; }
 // using namespace foo;
Index: clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
@@ -1,4 +1,4 @@
-//===-- ExpandAutoTypeTests.cpp -*- C++ -*-===//
+//===-- ExpandDeducedTypeTests.cpp --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,9 +16,9 @@
 namespace clangd {
 namespace {
 
-TWEAK_TEST(ExpandAutoType);
+TWEAK_TEST(ExpandDeducedType);
 
-TEST_F(ExpandAutoTypeTest, Test) {
+TEST_F(ExpandDeducedTypeTest, Test) {
   Header = R"cpp(
 namespace ns {
   struct Class {
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could not expand type of function pointer"));
+  StartsWith("fail: Could not expand type"));
+  // function references should not be replaced
+  EXPECT_THAT(apply("au^to  = ns::Func;"),
+  StartsWith("fail: Could not expand 

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 488265.
v1nh1shungry added a comment.

address the comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check-lines.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/test/code-action-request.test
  clang-tools-extra/clangd/test/request-reply.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -129,7 +129,7 @@
 "tweaks/DumpASTTests.cpp",
 "tweaks/DumpRecordLayoutTests.cpp",
 "tweaks/DumpSymbolTests.cpp",
-"tweaks/ExpandAutoTypeTests.cpp",
+"tweaks/ExpandDeducedTypeTests.cpp",
 "tweaks/ExpandMacroTests.cpp",
 "tweaks/ExtractFunctionTests.cpp",
 "tweaks/ExtractVariableTests.cpp",
Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -18,7 +18,7 @@
 "DefineInline.cpp",
 "DefineOutline.cpp",
 "DumpAST.cpp",
-"ExpandAutoType.cpp",
+"ExpandDeducedType.cpp",
 "ExpandMacro.cpp",
 "ExtractFunction.cpp",
 "ExtractVariable.cpp",
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -1611,7 +1611,7 @@
 clang-tools-extra/clangd/unittests/tweaks/DumpASTTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpRecordLayoutTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpSymbolTests.cpp
-clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExpandMacroTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -25,9 +25,9 @@
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
-// TWEAK_TEST(ExpandAutoType);
+// TWEAK_TEST(ExpandDeducedType);
 //
-// TEST_F(ExpandAutoTypeTest, ShortensTypes) {
+// TEST_F(ExpandDeducedTypeTest, ShortensTypes) {
 //   Header = R"cpp(
 // namespace foo { template class X{}; }
 // using namespace foo;
Index: clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
@@ -1,4 +1,4 @@
-//===-- ExpandAutoTypeTests.cpp -*- C++ -*-===//
+//===-- ExpandDeducedTypeTests.cpp --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,9 +16,9 @@
 namespace clangd {
 namespace {
 
-TWEAK_TEST(ExpandAutoType);
+TWEAK_TEST(ExpandDeducedType);
 
-TEST_F(ExpandAutoTypeTest, Test) {
+TEST_F(ExpandDeducedTypeTest, Test) {
   Header = R"cpp(
 namespace ns {
   struct Class {
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could not expand type of function pointer"));
+  StartsWith("fail: Could not expand type"));
+  // function references should not be replaced
+  EXPECT_THAT(apply("au^to  = ns::Func;"),
+  StartsWith("fail: Could not 

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:139
 
-  // if it's a lambda expression, return an error message
-  if (isa(*DeducedType) &&
-  cast(*DeducedType)->getDecl()->isLambda()) {
-return error("Could not expand type of lambda expression");
+  // we shouldn't replace a dependent type, e.g.
+  //   template 

sammccall wrote:
> v1nh1shungry wrote:
> > sammccall wrote:
> > > why not? replacing this with `T` seems perfectly reasonable.
> > > (The fact that we don't do this with `auto t = T{}` is just because we're 
> > > hitting a limitation of clang's AST)
> > I'd like it too. But the `printType()` would give out a ``. 
> > Though I haven't looked into this, would you mind giving some suggestions 
> > about it?
> Ah, there are three subtly different concepts here:
>  - is this type usefully printable? There are various reasons why it may not 
> be, it can contain DependentTy, or a canonical template parameter 
> (``), or a lambda.
>  - is this type dependent in the language sense - an example is some type 
> parameter `T`. This may or may not be usefully printable. (e.g. try 
> `template std::vector y;` and hover on y)
>  - is this type specifically `DependentTy`, the placeholder dependent type 
> which we have no information about. This is never usefully printable: prints 
> as ``
> 
> As a heuristic, I'm happy with saying dependent types (in the language sense) 
> are likely not to print usefully, so don't replace them. But the comment 
> should say so (this is a heuristic for unprintable rather than an end in 
> itself), and probably give examples.
Hmm, do you mean it's okay to refuse to replace dependent types but I should 
leave a comment saying the reason is that they are likely not to print 
usefully? Sorry if I misunderstood anything! I'm really not a good reader :(



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:151
+  //   ^^ is `int[10]`
+  if ((*DeducedType)->isArrayType())
+return error("Could not expand type of array");

sammccall wrote:
> the commonality between these cases you're excluding (and the function types 
> below) is that they use C-style declarator syntax that may have chunks 
> following the declarator-id. Specifically:
>  - array, function, and paren types always have chunks following the 
> declarator
>  - pointer and reference types compose inside-out so their pointee types may 
> contain trailing chunks
> 
> If we want to make some attempt to detect more cases, we should pull out a 
> function here and do it properly, something like: `bool 
> hasTrailingChunks(QualType)` which calls recursively for pointertype etc.
> 
> But there's a cheaper way: when we call `printType()` below, we can 
> optionally specify the declarator-id. Then we can simply check whether it's 
> at the end of the string:
> 
> ```
> std::string PrettyDeclarator = printType(..., "DECLARATOR_ID");
> llvm::StringRef PrettyType = PrettyDeclarator;
> if (!PrettyType.consume_back("DECLARATOR_ID"))
>   return error("could not expand non-contiguous type {0}", PrettyType);
> PrettyType = PrettyType.rtrim();
> ```
> 
> This feels slightly hacky, but it's direct and simple and we shouldn't miss a 
> case.
TBH, I'm confused about `printType()` and its placeholder `DECLARATOR_ID`. Is 
the code your offered above can be used directly?

If so, I had a try on it and it did refuse to replace types like function and 
array. But it would give an error message saying "Could not expand 
non-contiguous type const char (_ID)[6]". Is this okay? I mean isn't 
the "DECLARATOR_ID" in the message a bit confusing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

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

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thank you for reviewing and giving suggestions!




Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:57
+std::string ExpandDeducedType::title() const {
+  return IsAutoType ? "Expand auto type" : "Expand decltype";
+}

sammccall wrote:
> Maybe "Replace with deduced type" would be clearer for both cases?
> 
> Then we don't need to track IsAutoType.
> (I have no objection with spending a bit of code to provide better text, but 
> given that the cursor is either on top of "auto" or "decltype" I don't think 
> we're actually adding anything by echoing it back)
I think your suggestion is better. Thanks!



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:139
 
-  // if it's a lambda expression, return an error message
-  if (isa(*DeducedType) &&
-  cast(*DeducedType)->getDecl()->isLambda()) {
-return error("Could not expand type of lambda expression");
+  // we shouldn't replace a dependent type, e.g.
+  //   template 

sammccall wrote:
> why not? replacing this with `T` seems perfectly reasonable.
> (The fact that we don't do this with `auto t = T{}` is just because we're 
> hitting a limitation of clang's AST)
I'd like it too. But the `printType()` would give out a ``. 
Though I haven't looked into this, would you mind giving some suggestions about 
it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 488189.
v1nh1shungry added a comment.

add a release note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/DeclTemplate.h
  clang/test/FixIt/fixit-const-var-init.cpp


Index: clang/test/FixIt/fixit-const-var-init.cpp
===
--- /dev/null
+++ clang/test/FixIt/fixit-const-var-init.cpp
@@ -0,0 +1,28 @@
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++14 %s 
2>&1 | FileCheck %s
+
+const int a; // expected-error {{default initialization of an object of const 
type}}
+// CHECK: fix-it:"{{.*}}":{3:12-3:12}:" = 0"
+
+template  const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{6:36-6:36}:" = 0"
+
+template  const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{9:39-9:39}:" = 0"
+
+template <> const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{12:36-12:36}:" = 0"
+
+constexpr float c; // expected-error {{must be initialized by a constant 
expression}}
+// CHECK: fix-it:"{{.*}}":{15:18-15:18}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{18:42-18:42}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{21:45-21:45}:" = 0.0"
+
+template <> constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{24:42-24:42}:" = 0.0"
+
+void (* const func)(int, int); // expected-error {{default initialization of 
an object of const type}}
+// CHECK: fix-it:"{{.*}}":{27:30-27:30}:" = nullptr"
Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -2924,6 +2924,14 @@
 return ExplicitInfo ? ExplicitInfo->TemplateKeywordLoc : SourceLocation();
   }
 
+  SourceRange getSourceRange() const override LLVM_READONLY {
+if (isExplicitSpecialization()) {
+  if (const ASTTemplateArgumentListInfo *Info = getTemplateArgsInfo())
+return SourceRange(getOuterLocStart(), Info->getRAngleLoc());
+}
+return VarDecl::getSourceRange();
+  }
+
   void Profile(llvm::FoldingSetNodeID ) const {
 Profile(ID, TemplateArgs->asArray(), getASTContext());
   }
@@ -3081,6 +3089,14 @@
 return First->InstantiatedFromMember.setInt(true);
   }
 
+  SourceRange getSourceRange() const override LLVM_READONLY {
+if (isExplicitSpecialization()) {
+  if (const ASTTemplateArgumentListInfo *Info = getTemplateArgsAsWritten())
+return SourceRange(getOuterLocStart(), Info->getRAngleLoc());
+}
+return VarDecl::getSourceRange();
+  }
+
   void Profile(llvm::FoldingSetNodeID ) const {
 Profile(ID, getTemplateArgs().asArray(), getTemplateParameters(),
 getASTContext());
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -348,6 +348,8 @@
 - Fix issue that the standard C++ modules importer will call global
   constructor/destructor for the global varaibles in the importing modules.
   This fixes `Issue 59765 `_
+- Fix the bug of inserting the ``ZeroInitializationFixit`` before the template
+  argument list of ``VarTemplateSpecializationDecl``.
 
 Improvements to Clang's diagnostics
 ^^^


Index: clang/test/FixIt/fixit-const-var-init.cpp
===
--- /dev/null
+++ clang/test/FixIt/fixit-const-var-init.cpp
@@ -0,0 +1,28 @@
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++14 %s 2>&1 | FileCheck %s
+
+const int a; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{3:12-3:12}:" = 0"
+
+template  const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{6:36-6:36}:" = 0"
+
+template  const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{9:39-9:39}:" = 0"
+
+template <> const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{12:36-12:36}:" = 0"
+
+constexpr float c; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{15:18-15:18}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be initialized by a constant expression}}
+// CHECK: 

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thank you for reviewing! I have opened an issue on GitHub: 
https://github.com/llvm/llvm-project/issues/59935. Hope my description is 
appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thanks for the reply! I'll raise a GitHub issue.

> var will have three attributes associated with it, but the only source 
> location information you have access to is for the foo, bar, and baz tokens. 
> Each of those attributes also keeps track of what syntax was used, so you 
> could tell that foo was a GNU-style attribute while bar and baz were C++. But 
> we don't keep enough information about bar and baz being part of the same 
> attribute list or whether that attribute list is leading or trailing. You 
> have to calculate all of this yourself and some of it might not even be 
> possible to calculate (for example, the user could have done 
> __attribute__((foo)) const int var [[bar]] [[baz]]; and the AST would come 
> out the same as the previous example).

Can I post your reply there? I think it will help.

> and then we'll ignore attributes for this patch.

Then I think this patch is ready for a review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 488138.
v1nh1shungry added a comment.

add `LLVM_READONLY` to `getSourceRange()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

Files:
  clang/include/clang/AST/DeclTemplate.h
  clang/test/FixIt/fixit-const-var-init.cpp


Index: clang/test/FixIt/fixit-const-var-init.cpp
===
--- /dev/null
+++ clang/test/FixIt/fixit-const-var-init.cpp
@@ -0,0 +1,28 @@
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++14 %s 
2>&1 | FileCheck %s
+
+const int a; // expected-error {{default initialization of an object of const 
type}}
+// CHECK: fix-it:"{{.*}}":{3:12-3:12}:" = 0"
+
+template  const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{6:36-6:36}:" = 0"
+
+template  const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{9:39-9:39}:" = 0"
+
+template <> const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{12:36-12:36}:" = 0"
+
+constexpr float c; // expected-error {{must be initialized by a constant 
expression}}
+// CHECK: fix-it:"{{.*}}":{15:18-15:18}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{18:42-18:42}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{21:45-21:45}:" = 0.0"
+
+template <> constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{24:42-24:42}:" = 0.0"
+
+void (* const func)(int, int); // expected-error {{default initialization of 
an object of const type}}
+// CHECK: fix-it:"{{.*}}":{27:30-27:30}:" = nullptr"
Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -2924,6 +2924,14 @@
 return ExplicitInfo ? ExplicitInfo->TemplateKeywordLoc : SourceLocation();
   }
 
+  SourceRange getSourceRange() const override LLVM_READONLY {
+if (isExplicitSpecialization()) {
+  if (const ASTTemplateArgumentListInfo *Info = getTemplateArgsInfo())
+return SourceRange(getOuterLocStart(), Info->getRAngleLoc());
+}
+return VarDecl::getSourceRange();
+  }
+
   void Profile(llvm::FoldingSetNodeID ) const {
 Profile(ID, TemplateArgs->asArray(), getASTContext());
   }
@@ -3081,6 +3089,14 @@
 return First->InstantiatedFromMember.setInt(true);
   }
 
+  SourceRange getSourceRange() const override LLVM_READONLY {
+if (isExplicitSpecialization()) {
+  if (const ASTTemplateArgumentListInfo *Info = getTemplateArgsAsWritten())
+return SourceRange(getOuterLocStart(), Info->getRAngleLoc());
+}
+return VarDecl::getSourceRange();
+  }
+
   void Profile(llvm::FoldingSetNodeID ) const {
 Profile(ID, getTemplateArgs().asArray(), getTemplateParameters(),
 getASTContext());


Index: clang/test/FixIt/fixit-const-var-init.cpp
===
--- /dev/null
+++ clang/test/FixIt/fixit-const-var-init.cpp
@@ -0,0 +1,28 @@
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++14 %s 2>&1 | FileCheck %s
+
+const int a; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{3:12-3:12}:" = 0"
+
+template  const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{6:36-6:36}:" = 0"
+
+template  const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{9:39-9:39}:" = 0"
+
+template <> const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{12:36-12:36}:" = 0"
+
+constexpr float c; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{15:18-15:18}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{18:42-18:42}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{21:45-21:45}:" = 0.0"
+
+template <> constexpr float d; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{24:42-24:42}:" = 0.0"
+
+void (* const func)(int, int); // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{27:30-27:30}:" = nullptr"
Index: clang/include/clang/AST/DeclTemplate.h
===
--- 

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang/test/FixIt/fixit-const-var-init.cpp:24
+
+template <> constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{24:42-24:42}:" = 0.0"

aaron.ballman wrote:
> v1nh1shungry wrote:
> > aaron.ballman wrote:
> > > I'd like to see some additional test coverage for more complicated 
> > > declarators:
> > > ```
> > > void (* const func)(int, int);
> > > const int var [[clang::annotate("")]];
> > > ```
> > One more question: I just did the second test
> > 
> > ```
> > const int var [[clang::annotate("")]];
> > ```
> > 
> > and it inserted the fix hint after the identifier.
> > 
> > Is this the expected behavior of `getEndLoc()`?
> @erichkeane and I talked about this off-list because it's a bit of an 
> academic question as to whether an attribute is "part" of a declaration or 
> not. We ultimately decided that we think it's best for the attribute to *not* 
> be considered part of the source range of the variable declaration; they are 
> wholly separate AST nodes.
> 
> So based on that, we think you will need some changes to SemaInit.cpp after 
> all. And you'll have to use `SourceManager::isBeforeInTranslationUnit()` to 
> determine if the begin source location of the attribute is after the end 
> source location of the variable or not (because the attribute could go before 
> the type information or after the variable name).
> 
> There's another test case to consider that hopefully will Just Work with this 
> design, but is worth testing explicitly:
> ```
> void (* const func [[clang::annotate("test")]])(int, int);
> ```
I have trouble getting the location of the right bracket of the attribute list, 
which may be used to insert the fix hint. Could you please give me some hints? 
Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 488081.
v1nh1shungry added a comment.

implement `VarTemplateSpecializationDecl::getSourceRange()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

Files:
  clang/include/clang/AST/DeclTemplate.h
  clang/test/FixIt/fixit-const-var-init.cpp


Index: clang/test/FixIt/fixit-const-var-init.cpp
===
--- /dev/null
+++ clang/test/FixIt/fixit-const-var-init.cpp
@@ -0,0 +1,28 @@
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++14 %s 
2>&1 | FileCheck %s
+
+const int a; // expected-error {{default initialization of an object of const 
type}}
+// CHECK: fix-it:"{{.*}}":{3:12-3:12}:" = 0"
+
+template  const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{6:36-6:36}:" = 0"
+
+template  const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{9:39-9:39}:" = 0"
+
+template <> const int b; // expected-error {{default 
initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{12:36-12:36}:" = 0"
+
+constexpr float c; // expected-error {{must be initialized by a constant 
expression}}
+// CHECK: fix-it:"{{.*}}":{15:18-15:18}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{18:42-18:42}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{21:45-21:45}:" = 0.0"
+
+template <> constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{24:42-24:42}:" = 0.0"
+
+void (* const func)(int, int); // expected-error {{default initialization of 
an object of const type}}
+// CHECK: fix-it:"{{.*}}":{27:30-27:30}:" = nullptr"
Index: clang/include/clang/AST/DeclTemplate.h
===
--- clang/include/clang/AST/DeclTemplate.h
+++ clang/include/clang/AST/DeclTemplate.h
@@ -2924,6 +2924,14 @@
 return ExplicitInfo ? ExplicitInfo->TemplateKeywordLoc : SourceLocation();
   }
 
+  SourceRange getSourceRange() const override {
+if (isExplicitSpecialization()) {
+  if (const ASTTemplateArgumentListInfo *Info = getTemplateArgsInfo())
+return SourceRange(getOuterLocStart(), Info->getRAngleLoc());
+}
+return VarDecl::getSourceRange();
+  }
+
   void Profile(llvm::FoldingSetNodeID ) const {
 Profile(ID, TemplateArgs->asArray(), getASTContext());
   }
@@ -3081,6 +3089,14 @@
 return First->InstantiatedFromMember.setInt(true);
   }
 
+  SourceRange getSourceRange() const override {
+if (isExplicitSpecialization()) {
+  if (const ASTTemplateArgumentListInfo *Info = getTemplateArgsAsWritten())
+return SourceRange(getOuterLocStart(), Info->getRAngleLoc());
+}
+return VarDecl::getSourceRange();
+  }
+
   void Profile(llvm::FoldingSetNodeID ) const {
 Profile(ID, getTemplateArgs().asArray(), getTemplateParameters(),
 getASTContext());


Index: clang/test/FixIt/fixit-const-var-init.cpp
===
--- /dev/null
+++ clang/test/FixIt/fixit-const-var-init.cpp
@@ -0,0 +1,28 @@
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++14 %s 2>&1 | FileCheck %s
+
+const int a; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{3:12-3:12}:" = 0"
+
+template  const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{6:36-6:36}:" = 0"
+
+template  const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{9:39-9:39}:" = 0"
+
+template <> const int b; // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{12:36-12:36}:" = 0"
+
+constexpr float c; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{15:18-15:18}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{18:42-18:42}:" = 0.0"
+
+template  constexpr float d; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{21:45-21:45}:" = 0.0"
+
+template <> constexpr float d; // expected-error {{must be initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{24:42-24:42}:" = 0.0"
+
+void (* const func)(int, int); // expected-error {{default initialization of an object of const type}}
+// CHECK: fix-it:"{{.*}}":{27:30-27:30}:" = nullptr"
Index: clang/include/clang/AST/DeclTemplate.h
===
--- 

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang/test/FixIt/fixit-const-var-init.cpp:24
+
+template <> constexpr float d; // expected-error {{must be 
initialized by a constant expression}}
+// CHECK: fix-it:"{{.*}}":{24:42-24:42}:" = 0.0"

aaron.ballman wrote:
> I'd like to see some additional test coverage for more complicated 
> declarators:
> ```
> void (* const func)(int, int);
> const int var [[clang::annotate("")]];
> ```
One more question: I just did the second test

```
const int var [[clang::annotate("")]];
```

and it inserted the fix hint after the identifier.

Is this the expected behavior of `getEndLoc()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:3863
 
+  SourceLocation EndLoc = VD->getEndLoc();
+  if (const auto *VTSD = dyn_cast(VD)) {

aaron.ballman wrote:
> erichkeane wrote:
> > v1nh1shungry wrote:
> > > aaron.ballman wrote:
> > > > erichkeane wrote:
> > > > > Hmm... it is strange to me that the variables 'endloc' is the end of 
> > > > > the identifier and not the end of the variable declaration.  I 
> > > > > unfortunately don't have a good feeling as to the 'correct' behavior 
> > > > > for that (Aaron is typically the one who understands source locations 
> > > > > better than me!), so hopefully he can come along and see.  
> > > > I don't think we should have to do this dance here, this is something 
> > > > that `getEndLoc()` should be able to tell us. The way that source 
> > > > locations work is that AST nodes can override `getSourceRange()` to 
> > > > produce the correct range information for the node, and then they 
> > > > expose different accessors for any other source location information. 
> > > > In this case, I think `VarTemplateSpecializationDecl` isn't overloading 
> > > > `getSourceRange()` and so we're getting the default behavior from 
> > > > `VarDecl`.
> > > Sorry! I'm confused here. Do you mean we should use `getEndLoc()` 
> > > directly as the location where the fix-hint insert? But isn't the 
> > > original code using `getEndLoc()`, it turns out to add the fix-hint after 
> > > the end of the identifier instead of the right angle of 
> > > `VarTemplateSpecializationDecl`.
> > Right, we should use `getEndLoc` directly for hte fixit location, and 'fix' 
> > `getEndLoc` by having `VarTemplateSpecializationDecl` override 
> > `getSourceRange` to have the right `end loc`.
> > Sorry! I'm confused here. Do you mean we should use getEndLoc() directly as 
> > the location where the fix-hint insert? But isn't the original code using 
> > getEndLoc(), it turns out to add the fix-hint after the end of the 
> > identifier instead of the right angle of VarTemplateSpecializationDecl.
> 
> Thanks for asking for clarification! I think the code here in SemaInit.cpp is 
> correct as-is and we should instead be implementing 
> `VarTemplateSpecializationDecl::getSourceRange()` to calculate the correct 
> begin and end location for the variable declaration. This should fix 
> SemaInit.cpp because `getEndLoc()` is implemented by calling 
> `getSourceRange()` and returning the end location: 
> https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/DeclBase.h#L428
I see. Thanks for the clarification!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:3863
 
+  SourceLocation EndLoc = VD->getEndLoc();
+  if (const auto *VTSD = dyn_cast(VD)) {

aaron.ballman wrote:
> erichkeane wrote:
> > Hmm... it is strange to me that the variables 'endloc' is the end of the 
> > identifier and not the end of the variable declaration.  I unfortunately 
> > don't have a good feeling as to the 'correct' behavior for that (Aaron is 
> > typically the one who understands source locations better than me!), so 
> > hopefully he can come along and see.  
> I don't think we should have to do this dance here, this is something that 
> `getEndLoc()` should be able to tell us. The way that source locations work 
> is that AST nodes can override `getSourceRange()` to produce the correct 
> range information for the node, and then they expose different accessors for 
> any other source location information. In this case, I think 
> `VarTemplateSpecializationDecl` isn't overloading `getSourceRange()` and so 
> we're getting the default behavior from `VarDecl`.
Sorry! I'm confused here. Do you mean we should use `getEndLoc()` directly as 
the location where the fix-hint insert? But isn't the original code using 
`getEndLoc()`, it turns out to add the fix-hint after the end of the identifier 
instead of the right angle of `VarTemplateSpecializationDecl`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thanks for reviewing, @erichkeane! And a gentle ping to @aaron.ballman


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

> About my template example: I wanted to say that the actual 
> bugprone-implicit-widening-of-multiplication-result rule looks to not analyze 
> template calculation problem. So I think it's better to use the desugared 
> type (size_t).

Hmm, I don't think this two are related.

> It's not acceptable (IMHO) that the hard-coded size_t is resolved as long. 
> According to https://en.cppreference.com/w/cpp/language/types long may be 32 
> bits.

Yeah, I don't have a good feeling about it too. The point is that we love 
desugaring `size_type` to `size_t` (which I haven't found a way to achieve 
yet), and hate desugaring `int64_t` to `long`, but how can we classify them? 
They BOTH are the desugared type.

According to the conversation above, I'd use the qualified alias type name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 487704.
v1nh1shungry added a comment.

avoid expanding pointer to an array


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check-lines.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/test/code-action-request.test
  clang-tools-extra/clangd/test/request-reply.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -129,7 +129,7 @@
 "tweaks/DumpASTTests.cpp",
 "tweaks/DumpRecordLayoutTests.cpp",
 "tweaks/DumpSymbolTests.cpp",
-"tweaks/ExpandAutoTypeTests.cpp",
+"tweaks/ExpandDeducedTypeTests.cpp",
 "tweaks/ExpandMacroTests.cpp",
 "tweaks/ExtractFunctionTests.cpp",
 "tweaks/ExtractVariableTests.cpp",
Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -18,7 +18,7 @@
 "DefineInline.cpp",
 "DefineOutline.cpp",
 "DumpAST.cpp",
-"ExpandAutoType.cpp",
+"ExpandDeducedType.cpp",
 "ExpandMacro.cpp",
 "ExtractFunction.cpp",
 "ExtractVariable.cpp",
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -1611,7 +1611,7 @@
 clang-tools-extra/clangd/unittests/tweaks/DumpASTTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpRecordLayoutTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpSymbolTests.cpp
-clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExpandMacroTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -25,9 +25,9 @@
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
-// TWEAK_TEST(ExpandAutoType);
+// TWEAK_TEST(ExpandDeducedType);
 //
-// TEST_F(ExpandAutoTypeTest, ShortensTypes) {
+// TEST_F(ExpandDeducedTypeTest, ShortensTypes) {
 //   Header = R"cpp(
 // namespace foo { template class X{}; }
 // using namespace foo;
Index: clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
@@ -1,4 +1,4 @@
-//===-- ExpandAutoTypeTests.cpp -*- C++ -*-===//
+//===-- ExpandDeducedTypeTests.cpp --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,9 +16,9 @@
 namespace clangd {
 namespace {
 
-TWEAK_TEST(ExpandAutoType);
+TWEAK_TEST(ExpandDeducedType);
 
-TEST_F(ExpandAutoTypeTest, Test) {
+TEST_F(ExpandDeducedTypeTest, Test) {
   Header = R"cpp(
 namespace ns {
   struct Class {
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could not expand type of function pointer"));
+  StartsWith("fail: Could not expand type of function"));
+  // function references should not be replaced
+  EXPECT_THAT(apply("au^to  = ns::Func;"),
+  

[PATCH] D141226: [WIP][clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 487688.
v1nh1shungry added a comment.

- avoid expanding dependent types
- avoid expanding reference to a function
- avoid expanding array type and reference to an array


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check-lines.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/test/code-action-request.test
  clang-tools-extra/clangd/test/request-reply.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -129,7 +129,7 @@
 "tweaks/DumpASTTests.cpp",
 "tweaks/DumpRecordLayoutTests.cpp",
 "tweaks/DumpSymbolTests.cpp",
-"tweaks/ExpandAutoTypeTests.cpp",
+"tweaks/ExpandDeducedTypeTests.cpp",
 "tweaks/ExpandMacroTests.cpp",
 "tweaks/ExtractFunctionTests.cpp",
 "tweaks/ExtractVariableTests.cpp",
Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -18,7 +18,7 @@
 "DefineInline.cpp",
 "DefineOutline.cpp",
 "DumpAST.cpp",
-"ExpandAutoType.cpp",
+"ExpandDeducedType.cpp",
 "ExpandMacro.cpp",
 "ExtractFunction.cpp",
 "ExtractVariable.cpp",
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -1611,7 +1611,7 @@
 clang-tools-extra/clangd/unittests/tweaks/DumpASTTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpRecordLayoutTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpSymbolTests.cpp
-clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExpandMacroTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -25,9 +25,9 @@
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
-// TWEAK_TEST(ExpandAutoType);
+// TWEAK_TEST(ExpandDeducedType);
 //
-// TEST_F(ExpandAutoTypeTest, ShortensTypes) {
+// TEST_F(ExpandDeducedTypeTest, ShortensTypes) {
 //   Header = R"cpp(
 // namespace foo { template class X{}; }
 // using namespace foo;
Index: clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
@@ -1,4 +1,4 @@
-//===-- ExpandAutoTypeTests.cpp -*- C++ -*-===//
+//===-- ExpandDeducedTypeTests.cpp --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,9 +16,9 @@
 namespace clangd {
 namespace {
 
-TWEAK_TEST(ExpandAutoType);
+TWEAK_TEST(ExpandDeducedType);
 
-TEST_F(ExpandAutoTypeTest, Test) {
+TEST_F(ExpandDeducedTypeTest, Test) {
   Header = R"cpp(
 namespace ns {
   struct Class {
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could not expand type of function pointer"));
+  StartsWith("fail: Could not expand type of function"));
+  // function 

[PATCH] D141226: [WIP][clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 487519.
v1nh1shungry added a comment.
Herald added projects: clang, LLVM.
Herald added a subscriber: llvm-commits.

rename the tweak `ExpandAutoType` to `ExpandDeducedType`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check-lines.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/test/code-action-request.test
  clang-tools-extra/clangd/test/request-reply.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -129,7 +129,7 @@
 "tweaks/DumpASTTests.cpp",
 "tweaks/DumpRecordLayoutTests.cpp",
 "tweaks/DumpSymbolTests.cpp",
-"tweaks/ExpandAutoTypeTests.cpp",
+"tweaks/ExpandDeducedTypeTests.cpp",
 "tweaks/ExpandMacroTests.cpp",
 "tweaks/ExtractFunctionTests.cpp",
 "tweaks/ExtractVariableTests.cpp",
Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -18,7 +18,7 @@
 "DefineInline.cpp",
 "DefineOutline.cpp",
 "DumpAST.cpp",
-"ExpandAutoType.cpp",
+"ExpandDeducedType.cpp",
 "ExpandMacro.cpp",
 "ExtractFunction.cpp",
 "ExtractVariable.cpp",
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -1611,7 +1611,7 @@
 clang-tools-extra/clangd/unittests/tweaks/DumpASTTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpRecordLayoutTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpSymbolTests.cpp
-clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExpandMacroTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -25,9 +25,9 @@
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
-// TWEAK_TEST(ExpandAutoType);
+// TWEAK_TEST(ExpandDeducedType);
 //
-// TEST_F(ExpandAutoTypeTest, ShortensTypes) {
+// TEST_F(ExpandDeducedTypeTest, ShortensTypes) {
 //   Header = R"cpp(
 // namespace foo { template class X{}; }
 // using namespace foo;
Index: clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
@@ -1,4 +1,4 @@
-//===-- ExpandAutoTypeTests.cpp -*- C++ -*-===//
+//===-- ExpandDeducedTypeTests.cpp --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,9 +16,9 @@
 namespace clangd {
 namespace {
 
-TWEAK_TEST(ExpandAutoType);
+TWEAK_TEST(ExpandDeducedType);
 
-TEST_F(ExpandAutoTypeTest, Test) {
+TEST_F(ExpandDeducedTypeTest, Test) {
   Header = R"cpp(
 namespace ns {
   struct Class {
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could not expand type of function pointer"));
+  StartsWith("fail: Could not expand type of function"));
+  // function 

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

> There is a method QualType::getSingleStepDesugaredType() which may get size_t

Just did a quick test, and it doesn't work like this. So I can only get the 
fully desugared type for now. Sorry :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

> Sorry, I'm confused. Could you please explain what you expect this example 
> should achieve and what is "the rule"? Thanks!

Ah, I think I understand it. You mean the 
`bugprone-implicit-widening-of-multiplication-result` check doesn't warn 
anything about this example, right?

If so, although I don't understand what you mean by coming up with this 
example, to classify, when I said

> The second one will turn int64_t into long, which I don't have a good feeling 
> about.

the point is that if we choose to use the desugared type,

  int64_t foobar() {
return 1024 * 1024;
  }

will change to

  int64_t foobar() {
return static_cast(1024 * 1024);
    instead of int64_t
  }

Here I think this will break the portability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

> The rule does not complain.

Sorry, I'm confused. Could you please explain what you expect this example 
should achieve and what is "the rule"? Thanks!

> So I think that the "resolved" type (size_t) should be use instead of the 
> templated one (std::vector>::size_type).

Note: the "desugared type" I mention is the **FULLY** desugared type, so it 
will achieve `unsigned long` instead of `size_t`. There is a method 
`QualType::getSingleStepDesugaredType()` which may get `size_t`, but I haven't 
had a try. But even though it works, this will still turn `int64_t` into `long`.

BTW, if you think the desugared type is better, which do you think is better, 
to get single step desugared or to get fully desugared?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

> There are two approaches to fixing this issue. One is fixing with the FULLY 
> qualified type name, in the above case that is, std::vector std::allocator>::size_type. Another one is fixing with the desugared 
> type name, that is, unsigned long. I personally don't have a strong opinion 
> on which one is better.

IMHO, the first approach can cause redundancy in some cases, like this one. The 
second one will turn `int64_t` into `long`, which I don't have a good feeling 
about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

> For the second test, the type is the internal type of std::vector.

There are two approaches to fixing this issue. One is fixing with the **FULLY** 
qualified type name, in the above case that is, `std::vector>::size_type`. Another one is fixing with the desugared type 
name, that is, `unsigned long`. I personally don't have a strong opinion on 
which one is better.

WDYT, @bansan?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thank you for taking a look at this patch! @bansan

> For the first test, I still think that the auto fix should be return 
> static_cast(1024) * 1024 * 1024 * x; to have the good result.

I'd say I'm not the author of this checker. I think this behavior change needs 
a discussion somewhere, for example, you could raise an issue on GitHub. I just 
want to fix the wrong fix-hint and I think this is out of what this patch 
should do. Sorry!

> For the second test, the type is the internal type of std::vector.

Good catch, thanks! I didn't realize this problem. Will take a look at it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141226: [WIP][clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

If this is okay to go ahead, I'm considering renaming the tweak. Does 
`ExpandType` or `ExpandDeducedType` sound good?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

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

[PATCH] D141226: [WIP][clangd] support expanding `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Enable the existing code action `ExpandAutoType` to expand
`decltype(expr)`, e.g.

  decltype(0) i;

will expand to

  int i;


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could not expand type of function pointer"));
+  StartsWith("fail: Could not expand type of function"));
+  // function references should not be replaced
+  EXPECT_THAT(apply("au^to  = ns::Func;"),
+  StartsWith("fail: Could not expand type of function"));
   // lambda types are not replaced
   EXPECT_UNAVAILABLE("au^to x = []{};");
   // inline namespaces
@@ -78,6 +81,17 @@
   EXPECT_THAT(apply("template  void x() { ^auto y = T::z(); }"),
   StartsWith("fail: Could not deduce type for 'auto' type"));
 
+  EXPECT_EQ(apply("decl^type(0) i;"), "int i;");
+  EXPECT_THAT(apply("void f(); decl^type(f) g;"),
+  StartsWith("fail: Could not expand type of function"));
+  EXPECT_EQ(apply("decl^type(0) f();"), "int f();");
+  EXPECT_EQ(apply("auto f() -> decl^type(0) { return 0; }"),
+"auto f() -> int { return 0; }");
+  EXPECT_EQ(apply("template  class Foo {};"),
+"template  class Foo {};");
+  EXPECT_EQ(apply("template  class Bar {}; Bar b;"),
+"template  class Bar {}; Bar b;");
+
   ExtraArgs.push_back("-std=c++20");
   EXPECT_UNAVAILABLE("template  class Y;");
 
@@ -90,6 +104,9 @@
   // FIXME: should work on constrained auto params, once SourceRange is fixed.
   EXPECT_UNAVAILABLE("template concept C = true;"
  "auto X = [](C ^auto *){return 0;};");
+
+  EXPECT_UNAVAILABLE("auto f = [](){}; decl^type(f) g;");
+  EXPECT_UNAVAILABLE("decl^type([]{}) f;");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
@@ -29,7 +29,6 @@
 /// After:
 ///MyClass x = Something();
 ///^^^
-/// FIXME: Handle decltype as well
 class ExpandAutoType : public Tweak {
 public:
   const char *id() const final;
@@ -41,12 +40,15 @@
   std::string title() const override;
 
 private:
-  SourceRange AutoRange;
+  SourceRange Range;
+  bool IsAutoType;
 };
 
 REGISTER_TWEAK(ExpandAutoType)
 
-std::string ExpandAutoType::title() const { return "Expand auto type"; }
+std::string ExpandAutoType::title() const {
+  return IsAutoType ? "Expand auto type" : "Expand decltype";
+}
 
 // Structured bindings must use auto, e.g. `const auto& [a,b,c] = ...;`.
 // Return whether N (an AutoTypeLoc) is such an auto that must not be expanded.
@@ -93,7 +95,7 @@
 if (!isStructuredBindingType(Node) &&
 !isDeducedAsLambda(Node, Result.getBeginLoc()) &&
 !isTemplateParam(Node))
-  AutoRange = Result.getSourceRange();
+  Range = Result.getSourceRange();
   }
   if (auto TTPAuto = TypeNode->getAs()) {
 // We exclude concept constraints for now, as the SourceRange is wrong.
@@ -102,41 +104,43 @@
 // TTPAuto->getSourceRange only covers "auto", not "C auto".
 if (TTPAuto.getDecl()->isImplicit() &&
 !TTPAuto.getDecl()->hasTypeConstraint())
-  AutoRange = TTPAuto.getSourceRange();
+  Range = TTPAuto.getSourceRange();
+  }
+  IsAutoType = true;
+
+  if (auto DTTL = TypeNode->getAs()) {
+if (!isDeducedAsLambda(Node, DTTL.getBeginLoc()))
+  Range = DTTL.getSourceRange();
+IsAutoType = false;
   }
 }
   }
 
-  return AutoRange.isValid();
+  return Range.isValid();
 }
 
-Expected ExpandAutoType::apply(const Selection& Inputs) {
+Expected ExpandAutoType::apply(const Selection ) {
   auto  = Inputs.AST->getSourceManager();
 
   std::optional DeducedType =
-  getDeducedType(Inputs.AST->getASTContext(), AutoRange.getBegin());
+  getDeducedType(Inputs.AST->getASTContext(), Range.getBegin());
 
   // if we can't resolve the type, return an error message
   if (DeducedType == std::nullopt || 

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thank you for reviewing and giving suggestions! @Febbe

Please take a look at my reply. Sorry if I misunderstood anything!




Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:215
   (Twine("static_cast<") + TyAsString + ">(").str())
-   << FixItHint::CreateInsertion(IndexExpr->getEndLoc(), ")");
 else

Febbe wrote:
> Actually, I find it pretty weird/suspicious, that ` -> 
> getEndLoc()` does not return the end of its `Expr`. The code looked correct 
> already.
> Can you elaborate, if this is a bug in the `getEndLoc()` of those `Expr`s? It 
> might be better to fix it there directly.
> 
I'm really not an expert on `SourceLocation`. I just took a rough look at these 
`getEndLoc()`s.

Take `1 + 2` as an example, this is a `BinaryOperator` with two 
`IntegerLiteral`s. When we call `Expr::getEndLoc()` which is actually 
`Stmt::getEndLoc()` on it, it will call `BinaryOperator::getEndLoc()` and this 
actually returns the `RHS->getEndLoc()`, that is  the result of 
`IntegerLiteral(2)->getEndLoc()`. And the interesting stuff is that 
`IntegerLiteral::getBeginLoc()` and `IntegerLiteral::getEndLoc()` return the 
same `Loc`. Although I'm not sure which location these will return, I guess 
that's why `BinaryOperator::getEndLoc()` doesn't return the end of the 
expression.

Sorry if I misunderstood anything! After all, I didn't find documents saying 
which location these `Expr`s' `getEndLoc()` should return. I think it's better 
to have experts take a look at this.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp:18
   // CHECK-NOTES-C:(ptrdiff_t)( )
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:16: note: perform multiplication in a wider 
type

Actually I have tried adding

```
// CHECK-FIXES-C: return [(ptrdiff_t)(a * b)];
// CHECK-FIXES-CXX: return [static_cast(a * b)];
```

under this line, but the test failed, and when I took a look at 
`build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/implicit-widening-of-multiplication-result-array-subscript-expression.cpp.tmp.cpp`,
 I found that these codes didn't get modified. And I took a look at other files 
which have `CHECK-FIXES` lines, I found the codes in the corresponding 
temporary files got fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty

[Vincent Hong via Phabricator via cfe-commits]

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

Thank you for reviewing and giving suggestions! @cjdb

Hope there are enough test cases now, and not too many tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141107

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

[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 487044.
v1nh1shungry added a comment.

add more tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141107

Files:
  clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
@@ -155,7 +155,7 @@
 } // namespace qualifiers
 
 
-void test_member_empty() {
+bool test_member_empty() {
   {
 std::vector v;
 v.empty();
@@ -231,9 +231,69 @@
 s.empty();
 // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty]
   }
+
+  {
+std::vector v;
+return v.empty();
+// no-warning
+  }
+
+  {
+std::vector_with_clear v;
+return v.empty();
+// no-warning
+  }
+
+  {
+std::vector_with_int_empty v;
+return v.empty();
+// no-warning
+  }
+
+  {
+std::vector_with_clear_args v;
+return v.empty();
+// no-warning
+  }
+
+  {
+std::vector_with_clear_variable v;
+return v.empty();
+// no-warning
+  }
+
+  {
+absl::string s;
+return s.empty();
+// no-warning
+  }
+
+  {
+absl::string_with_clear s;
+return s.empty();
+// no-warning
+  }
+
+  {
+absl::string_with_int_empty s;
+return s.empty();
+// no-warning
+  }
+
+  {
+absl::string_with_clear_args s;
+return s.empty();
+// no-warning
+  }
+
+  {
+absl::string_with_clear_variable s;
+return s.empty();
+// no-warning
+  }
 }
 
-void test_qualified_empty() {
+bool test_qualified_empty() {
   {
 absl::string_with_clear v;
 std::empty(v);
@@ -260,9 +320,30 @@
 absl::empty(nullptr);
 // no-warning
   }
+
+  {
+absl::string_with_clear s;
+return std::empty(s);
+// no-warning
+return absl::empty(s);
+// no-warning
+  }
+
+  {
+absl::string s;
+return std::empty(s);
+// no-warning
+  }
+
+  {
+return std::empty(0);
+// no-warning
+return absl::empty(nullptr);
+// no-warning
+  }
 }
 
-void test_unqualified_empty() {
+bool test_unqualified_empty() {
   {
 std::vector v;
 empty(v);
@@ -370,6 +451,106 @@
 // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'absl::empty'; did you mean 'clear()'? [bugprone-standalone-empty]
 // CHECK-FIXES: {{^  }}  s.clear();{{$}}
   }
+
+  {
+std::vector v;
+return empty(v);
+// no-warning
+  }
+
+  {
+std::vector_with_void_empty v;
+return empty(v);
+// no-warning
+  }
+
+  {
+std::vector_with_clear v;
+return empty(v);
+// no-warning
+  }
+
+  {
+std::vector_with_int_empty v;
+return empty(v);
+// no-warning
+  }
+
+  {
+std::vector_with_clear_args v;
+return empty(v);
+// no-warning
+  }
+
+  {
+std::vector_with_clear_variable v;
+return empty(v);
+// no-warning
+  }
+
+  {
+absl::string s;
+return empty(s);
+// no-warning
+  }
+
+  {
+absl::string_with_void_empty s;
+return empty(s);
+// no-warning
+  }
+
+  {
+absl::string_with_clear s;
+return empty(s);
+// no-warning
+  }
+
+  {
+absl::string_with_int_empty s;
+return empty(s);
+// no-warning
+  }
+
+  {
+absl::string_with_clear_args s;
+return empty(s);
+// no-warning
+  }
+
+  {
+absl::string_with_clear_variable s;
+return empty(s);
+// no-warning
+  }
+
+  {
+std::vector v;
+using std::empty;
+return empty(v);
+// no-warning
+  }
+
+  {
+std::vector_with_clear v;
+using std::empty;
+return empty(v);
+// no-warning
+  }
+
+  {
+absl::string s;
+using absl::empty;
+return empty(s);
+// no-warning
+  }
+
+  {
+absl::string_with_clear s;
+using absl::empty;
+return empty(s);
+// no-warning
+  }
 }
 
 void test_empty_method_expressions() {
@@ -444,8 +625,7 @@
   // CHECK-MESSAGES: :[[#@LINE-1]]:27: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty]
 }
 
-void test_clear_in_base_class() {
-
+bool test_clear_in_base_class() {
   {
 base::vector v;
 v.empty();
@@ -497,9 +677,57 @@
 empty(v);
 // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'base::empty' [bugprone-standalone-empty]
   }
+
+  {
+base::vector v;
+return v.empty();
+// no-warning
+  }
+
+  {
+base::vector_non_dependent v;
+return v.empty();
+// no-warning
+  }
+
+  {
+base::vector_clear_with_args v;
+return v.empty();
+// no-warning
+  }
+
+  {
+base::vector_clear_variable v;
+return v.empty();
+// no-warning
+  }
+
+  {
+base::vector v;
+return 

[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added reviewers: cjdb, hokein.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Relevant issue: https://github.com/llvm/llvm-project/issues/59517

Currently this check will warn when the result is used in a `return`
statement, e.g.

  bool foobar() {
std::vector v;
return v.empty();
// will get a warning here, which makes no sense IMO
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141107

Files:
  clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
@@ -576,3 +576,16 @@
 // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 
'std::empty' [bugprone-standalone-empty]
   }
 }
+
+bool test_empty_in_return() {
+  {
+std::vector v;
+return v.empty();
+// no-warning
+  }
+  {
+std::vector v;
+return std::empty(v);
+// no-warning
+  }
+}
Index: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
@@ -98,6 +98,7 @@
   const auto PParentStmtExpr = Result.Nodes.getNodeAs("stexpr");
   const auto ParentCompStmt = Result.Nodes.getNodeAs("parent");
   const auto *ParentCond = getCondition(Result.Nodes, "parent");
+  const auto *ParentReturnStmt = Result.Nodes.getNodeAs("parent");
 
   if (const auto *MemberCall =
   Result.Nodes.getNodeAs("empty")) {
@@ -109,6 +110,9 @@
 if (PParentStmtExpr && ParentCompStmt &&
 ParentCompStmt->body_back() == MemberCall->getExprStmt())
   return;
+// Skip if it's a return statement
+if (ParentReturnStmt)
+  return;
 
 SourceLocation MemberLoc = MemberCall->getBeginLoc();
 SourceLocation ReplacementLoc = MemberCall->getExprLoc();
@@ -150,6 +154,8 @@
 if (PParentStmtExpr && ParentCompStmt &&
 ParentCompStmt->body_back() == NonMemberCall->getExprStmt())
   return;
+if (ParentReturnStmt)
+  return;
 
 SourceLocation NonMemberLoc = NonMemberCall->getExprLoc();
 SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc();


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
@@ -576,3 +576,16 @@
 // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty]
   }
 }
+
+bool test_empty_in_return() {
+  {
+std::vector v;
+return v.empty();
+// no-warning
+  }
+  {
+std::vector v;
+return std::empty(v);
+// no-warning
+  }
+}
Index: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
@@ -98,6 +98,7 @@
   const auto PParentStmtExpr = Result.Nodes.getNodeAs("stexpr");
   const auto ParentCompStmt = Result.Nodes.getNodeAs("parent");
   const auto *ParentCond = getCondition(Result.Nodes, "parent");
+  const auto *ParentReturnStmt = Result.Nodes.getNodeAs("parent");
 
   if (const auto *MemberCall =
   Result.Nodes.getNodeAs("empty")) {
@@ -109,6 +110,9 @@
 if (PParentStmtExpr && ParentCompStmt &&
 ParentCompStmt->body_back() == MemberCall->getExprStmt())
   return;
+// Skip if it's a return statement
+if (ParentReturnStmt)
+  return;
 
 SourceLocation MemberLoc = MemberCall->getBeginLoc();
 SourceLocation ReplacementLoc = MemberCall->getExprLoc();
@@ -150,6 +154,8 @@
 if (PParentStmtExpr && ParentCompStmt &&
 ParentCompStmt->body_back() == NonMemberCall->getExprStmt())
   return;
+if (ParentReturnStmt)
+  return;
 
 SourceLocation NonMemberLoc = NonMemberCall->getExprLoc();
 SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry marked 2 inline comments as done.
v1nh1shungry added a comment.

Thank you for reviewing and giving suggestions! @Eugene.Zelenko


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 486729.
v1nh1shungry added a comment.

don't use `auto`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

Files:
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
@@ -18,7 +18,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:17: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(ptrdiff_t)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 char *t1(char *base, int a, int b) {
   return a * b + base;
@@ -35,7 +35,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:17: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(size_t)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 
 char *t3(char *base, int a, unsigned int b) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
@@ -18,7 +18,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(long)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 unsigned long t1(int a, int b) {
   return a * b;
@@ -28,7 +28,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(long)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 
 long t2(unsigned int a, int b) {
@@ -39,7 +39,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(unsigned long)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 unsigned long t3(unsigned int a, int b) {
   return a * b;
@@ -49,7 +49,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(unsigned long)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 
 long t4(int a, unsigned int b) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
@@ -18,7 +18,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:16: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(ptrdiff_t)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 void *t1(char *base, int a, int b) {
   return &((a * b)[base]);
@@ -35,7 +35,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:16: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(size_t)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 
 char *t3(char *base, int a, unsigned int b) {
Index: 

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Sorry, I don't know how to add tests for such fixes. Could anyone please give 
me some hints? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141058

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

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added reviewers: lebedev.ri, ymandel.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Relevant issue: https://github.com/llvm/llvm-project/issues/56728


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141058

Files:
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
@@ -18,7 +18,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:17: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(ptrdiff_t)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 char *t1(char *base, int a, int b) {
   return a * b + base;
@@ -35,7 +35,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:17: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(size_t)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 
 char *t3(char *base, int a, unsigned int b) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
@@ -18,7 +18,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(long)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 unsigned long t1(int a, int b) {
   return a * b;
@@ -28,7 +28,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(long)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 
 long t2(unsigned int a, int b) {
@@ -39,7 +39,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(unsigned long)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 unsigned long t3(unsigned int a, int b) {
   return a * b;
@@ -49,7 +49,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(unsigned long)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 
 long t4(int a, unsigned int b) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
@@ -18,7 +18,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:16: note: perform multiplication in a wider type
   // CHECK-NOTES-C:(ptrdiff_t)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )
 }
 void *t1(char *base, int a, int b) {
   return &((a * b)[base]);
@@ -35,7 +35,7 @@
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:16: note: perform multiplication in a wider type
   // CHECK-NOTES-C: 

[PATCH] D140814: [clangd] show underlying type in type hint for `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 485923.
v1nh1shungry added a comment.

move the comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140814

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1364,7 +1364,6 @@
 TEST(TypeHints, Decltype) {
   assertTypeHints(R"cpp(
 $a[[decltype(0)]] a;
-// FIXME: will be nice to show `: int` instead
 $b[[decltype(a)]] b;
 const $c[[decltype(0)]]  = b;
 
@@ -1377,11 +1376,13 @@
 
 template  struct Foo;
 using G = Foo<$g[[decltype(0)]], float>;
+
+auto $h[[h]] = $i[[decltype(0)]]{};
   )cpp",
-  ExpectedHint{": int", "a"},
-  ExpectedHint{": decltype(0)", "b"},
+  ExpectedHint{": int", "a"}, ExpectedHint{": int", "b"},
   ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
-  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"});
+  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"},
+  ExpectedHint{": int", "h"}, ExpectedHint{": int", "i"});
 }
 
 TEST(DesignatorHints, Basic) {
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -662,7 +662,21 @@
  sourceLocToPosition(SM, Spelled->back().endLocation())};
   }
 
+  static bool shouldPrintCanonicalType(QualType QT) {
+// The sugared type is more useful in some cases, and the canonical
+// type in other cases. For now, prefer the sugared type unless
+// we are printing `decltype(expr)`. This could be refined further
+// (see https://github.com/clangd/clangd/issues/1298).
+if (QT->isDecltypeType())
+  return true;
+if (const AutoType *AT = QT->getContainedAutoType())
+  if (AT->getDeducedType()->isDecltypeType())
+return true;
+return false;
+  }
+
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
+TypeHintPolicy.PrintCanonicalTypes = shouldPrintCanonicalType(T);
 addTypeHint(R, T, Prefix, TypeHintPolicy);
   }
 


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1364,7 +1364,6 @@
 TEST(TypeHints, Decltype) {
   assertTypeHints(R"cpp(
 $a[[decltype(0)]] a;
-// FIXME: will be nice to show `: int` instead
 $b[[decltype(a)]] b;
 const $c[[decltype(0)]]  = b;
 
@@ -1377,11 +1376,13 @@
 
 template  struct Foo;
 using G = Foo<$g[[decltype(0)]], float>;
+
+auto $h[[h]] = $i[[decltype(0)]]{};
   )cpp",
-  ExpectedHint{": int", "a"},
-  ExpectedHint{": decltype(0)", "b"},
+  ExpectedHint{": int", "a"}, ExpectedHint{": int", "b"},
   ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
-  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"});
+  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"},
+  ExpectedHint{": int", "h"}, ExpectedHint{": int", "i"});
 }
 
 TEST(DesignatorHints, Basic) {
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -662,7 +662,21 @@
  sourceLocToPosition(SM, Spelled->back().endLocation())};
   }
 
+  static bool shouldPrintCanonicalType(QualType QT) {
+// The sugared type is more useful in some cases, and the canonical
+// type in other cases. For now, prefer the sugared type unless
+// we are printing `decltype(expr)`. This could be refined further
+// (see https://github.com/clangd/clangd/issues/1298).
+if (QT->isDecltypeType())
+  return true;
+if (const AutoType *AT = QT->getContainedAutoType())
+  if (AT->getDeducedType()->isDecltypeType())
+return true;
+return false;
+  }
+
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
+TypeHintPolicy.PrintCanonicalTypes = shouldPrintCanonicalType(T);
 addTypeHint(R, T, Prefix, TypeHintPolicy);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140814: [clangd] show underlying type in type hint for `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

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

Thanks for reviewing and giving suggestions!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140814

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

[PATCH] D140814: [clangd] show underlying type in type hint for `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 485854.
v1nh1shungry added a comment.

apply comment's suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140814

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1364,7 +1364,6 @@
 TEST(TypeHints, Decltype) {
   assertTypeHints(R"cpp(
 $a[[decltype(0)]] a;
-// FIXME: will be nice to show `: int` instead
 $b[[decltype(a)]] b;
 const $c[[decltype(0)]]  = b;
 
@@ -1377,11 +1376,13 @@
 
 template  struct Foo;
 using G = Foo<$g[[decltype(0)]], float>;
+
+auto $h[[h]] = $i[[decltype(0)]]{};
   )cpp",
-  ExpectedHint{": int", "a"},
-  ExpectedHint{": decltype(0)", "b"},
+  ExpectedHint{": int", "a"}, ExpectedHint{": int", "b"},
   ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
-  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"});
+  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"},
+  ExpectedHint{": int", "h"}, ExpectedHint{": int", "i"});
 }
 
 TEST(DesignatorHints, Basic) {
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -662,7 +662,21 @@
  sourceLocToPosition(SM, Spelled->back().endLocation())};
   }
 
+  // The sugared type is more useful in some cases, and the canonical
+  // type in other cases. For now, prefer the sugared type unless
+  // we are printing `decltype(expr)`. This could be refined further
+  // (see https://github.com/clangd/clangd/issues/1298).
+  static bool shouldPrintCanonicalType(QualType QT) {
+if (QT->isDecltypeType())
+  return true;
+if (const AutoType *AT = QT->getContainedAutoType())
+  if (AT->getDeducedType()->isDecltypeType())
+return true;
+return false;
+  }
+
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
+TypeHintPolicy.PrintCanonicalTypes = shouldPrintCanonicalType(T);
 addTypeHint(R, T, Prefix, TypeHintPolicy);
   }
 


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1364,7 +1364,6 @@
 TEST(TypeHints, Decltype) {
   assertTypeHints(R"cpp(
 $a[[decltype(0)]] a;
-// FIXME: will be nice to show `: int` instead
 $b[[decltype(a)]] b;
 const $c[[decltype(0)]]  = b;
 
@@ -1377,11 +1376,13 @@
 
 template  struct Foo;
 using G = Foo<$g[[decltype(0)]], float>;
+
+auto $h[[h]] = $i[[decltype(0)]]{};
   )cpp",
-  ExpectedHint{": int", "a"},
-  ExpectedHint{": decltype(0)", "b"},
+  ExpectedHint{": int", "a"}, ExpectedHint{": int", "b"},
   ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
-  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"});
+  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"},
+  ExpectedHint{": int", "h"}, ExpectedHint{": int", "i"});
 }
 
 TEST(DesignatorHints, Basic) {
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -662,7 +662,21 @@
  sourceLocToPosition(SM, Spelled->back().endLocation())};
   }
 
+  // The sugared type is more useful in some cases, and the canonical
+  // type in other cases. For now, prefer the sugared type unless
+  // we are printing `decltype(expr)`. This could be refined further
+  // (see https://github.com/clangd/clangd/issues/1298).
+  static bool shouldPrintCanonicalType(QualType QT) {
+if (QT->isDecltypeType())
+  return true;
+if (const AutoType *AT = QT->getContainedAutoType())
+  if (AT->getDeducedType()->isDecltypeType())
+return true;
+return false;
+  }
+
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
+TypeHintPolicy.PrintCanonicalTypes = shouldPrintCanonicalType(T);
 addTypeHint(R, T, Prefix, TypeHintPolicy);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140838: [clang] fix crash on generic lambda with lambda in decltype

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added reviewers: aaron.ballman, shafik.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Relevant issue: https://github.com/llvm/llvm-project/issues/59771

During the instantiation of a generic lambda, a non-generic lambda in
the trailing `decltype` is a `DeclContext` but not a dependent context,
so we shouldn't call `PerformDependentDiagnostics` on it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140838

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaCXX/lambda-unevaluated.cpp


Index: clang/test/SemaCXX/lambda-unevaluated.cpp
===
--- clang/test/SemaCXX/lambda-unevaluated.cpp
+++ clang/test/SemaCXX/lambda-unevaluated.cpp
@@ -144,3 +144,7 @@
 using d = decltype(sizeof([] static { return 0; }));
 
 }
+
+namespace lambda_in_trailing_decltype {
+auto x = ([](auto) -> decltype([] {}()) {}(0), 2);
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1230,7 +1230,7 @@
 
   // We recreated a local declaration, but not by instantiating it. There
   // may be pending dependent diagnostics to produce.
-  if (auto *DC = dyn_cast(Old))
+  if (auto *DC = dyn_cast(Old); DC && 
DC->isDependentContext())
 SemaRef.PerformDependentDiagnostics(DC, TemplateArgs);
 }
 


Index: clang/test/SemaCXX/lambda-unevaluated.cpp
===
--- clang/test/SemaCXX/lambda-unevaluated.cpp
+++ clang/test/SemaCXX/lambda-unevaluated.cpp
@@ -144,3 +144,7 @@
 using d = decltype(sizeof([] static { return 0; }));
 
 }
+
+namespace lambda_in_trailing_decltype {
+auto x = ([](auto) -> decltype([] {}()) {}(0), 2);
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1230,7 +1230,7 @@
 
   // We recreated a local declaration, but not by instantiating it. There
   // may be pending dependent diagnostics to produce.
-  if (auto *DC = dyn_cast(Old))
+  if (auto *DC = dyn_cast(Old); DC && DC->isDependentContext())
 SemaRef.PerformDependentDiagnostics(DC, TemplateArgs);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140814: [clangd] show underlying type in type hint for `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added a reviewer: nridge.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140814

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1364,7 +1364,6 @@
 TEST(TypeHints, Decltype) {
   assertTypeHints(R"cpp(
 $a[[decltype(0)]] a;
-// FIXME: will be nice to show `: int` instead
 $b[[decltype(a)]] b;
 const $c[[decltype(0)]]  = b;
 
@@ -1377,11 +1376,13 @@
 
 template  struct Foo;
 using G = Foo<$g[[decltype(0)]], float>;
+
+auto $h[[h]] = $i[[decltype(0)]]{};
   )cpp",
-  ExpectedHint{": int", "a"},
-  ExpectedHint{": decltype(0)", "b"},
+  ExpectedHint{": int", "a"}, ExpectedHint{": int", "b"},
   ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
-  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"});
+  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"},
+  ExpectedHint{": int", "h"}, ExpectedHint{": int", "i"});
 }
 
 TEST(DesignatorHints, Basic) {
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -663,7 +663,14 @@
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
+// print the underlying type for `decltype(expr)`
+if (T->isDecltypeType())
+  TypeHintPolicy.PrintCanonicalTypes = true;
+else if (const AutoType *AT = T->getContainedAutoType();
+ AT && AT->getDeducedType()->isDecltypeType())
+  TypeHintPolicy.PrintCanonicalTypes = true;
 addTypeHint(R, T, Prefix, TypeHintPolicy);
+TypeHintPolicy.PrintCanonicalTypes = false;
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix,


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1364,7 +1364,6 @@
 TEST(TypeHints, Decltype) {
   assertTypeHints(R"cpp(
 $a[[decltype(0)]] a;
-// FIXME: will be nice to show `: int` instead
 $b[[decltype(a)]] b;
 const $c[[decltype(0)]]  = b;
 
@@ -1377,11 +1376,13 @@
 
 template  struct Foo;
 using G = Foo<$g[[decltype(0)]], float>;
+
+auto $h[[h]] = $i[[decltype(0)]]{};
   )cpp",
-  ExpectedHint{": int", "a"},
-  ExpectedHint{": decltype(0)", "b"},
+  ExpectedHint{": int", "a"}, ExpectedHint{": int", "b"},
   ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
-  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"});
+  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"},
+  ExpectedHint{": int", "h"}, ExpectedHint{": int", "i"});
 }
 
 TEST(DesignatorHints, Basic) {
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -663,7 +663,14 @@
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
+// print the underlying type for `decltype(expr)`
+if (T->isDecltypeType())
+  TypeHintPolicy.PrintCanonicalTypes = true;
+else if (const AutoType *AT = T->getContainedAutoType();
+ AT && AT->getDeducedType()->isDecltypeType())
+  TypeHintPolicy.PrintCanonicalTypes = true;
 addTypeHint(R, T, Prefix, TypeHintPolicy);
+TypeHintPolicy.PrintCanonicalTypes = false;
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137817: [clangd] Improve action `RemoveUsingNamespace` for user-defined literals

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry abandoned this revision.
v1nh1shungry added a comment.

Abandon because of the terrible implementation.

- The cost seems to be higher than the value.

- The place to insert isn't good enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137817

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

[PATCH] D137817: [clangd] Improve action `RemoveUsingNamespace` for user-defined literals

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 484170.
v1nh1shungry added a comment.

Insert the using-declarations in the outermost `CompoundStmt` instead of
the innermost one. Can reduce some potential rebundancy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137817

Files:
  clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
  clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
@@ -250,20 +250,34 @@
 foo + 10;
   }
 )cpp"},
-  {// Does not qualify user-defined literals
-   R"cpp(
-  namespace ns {
-  long double operator "" _w(long double);
+  {
+  R"cpp(
+  namespace a {
+  long double operator""_a(long double);
+  inline namespace b {
+  long double operator""_b(long double);
+  } // namespace b
+  } // namespace a
+  using namespace ^a;
+  int main() {
+1.0_a;
+1.0_b;
   }
-  using namespace n^s;
-  int main() { 1.5_w; }
 )cpp",
-   R"cpp(
-  namespace ns {
-  long double operator "" _w(long double);
-  }
+  R"cpp(
+  namespace a {
+  long double operator""_a(long double);
+  inline namespace b {
+  long double operator""_b(long double);
+  } // namespace b
+  } // namespace a
   
-  int main() { 1.5_w; }
+  int main() {using a::operator""_a;
+using a::operator""_b;
+
+1.0_a;
+1.0_b;
+  }
 )cpp"}};
   for (auto C : Cases)
 EXPECT_EQ(C.second, apply(C.first)) << C.first;
Index: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
@@ -13,6 +13,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -101,6 +102,66 @@
   return D;
 }
 
+void handleUserDefinedLiterals(
+Decl *D, const llvm::SmallPtrSet ,
+std::map> , ASTContext ) {
+  struct Visitor : RecursiveASTVisitor {
+Visitor(const llvm::SmallPtrSet ,
+std::map> ,
+ASTContext )
+: Literals{Literals}, Result{Result}, Ctx{Ctx} {}
+
+const llvm::SmallPtrSet 
+std::map> 
+ASTContext 
+
+bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+  const NamedDecl *ND = DRE->getFoundDecl();
+  // If DRE references to a user-defined literal
+  if (ND->getDeclName().getNameKind() ==
+  DeclarationName::CXXLiteralOperatorName) {
+for (const NamedDecl *Literal : Literals) {
+  if (ND == Literal) {
+// If the user-defined literal locates in a CompoundStmt,
+// we mark the CompoundStmt so that we can add the using-declaration
+// in it later. If not, the user-defined literal is used in the
+// top-level, it is reasonable to do nothing for it.
+if (const CompoundStmt *CS = getCompoundParent(DRE))
+  Result[CS->getLBracLoc()].insert(ND->getQualifiedNameAsString());
+return true;
+  }
+}
+  }
+  return true;
+}
+
+// Get the outermost CompoundStmt parent of DRE
+const CompoundStmt *getCompoundParent(DeclRefExpr *DRE) {
+  const CompoundStmt *R = nullptr;
+
+  DynTypedNodeList Parents = Ctx.getParents(*DRE);
+  llvm::SmallVector NodesToProcess{Parents.begin(),
+ Parents.end()};
+
+  while (!NodesToProcess.empty()) {
+DynTypedNode Node = NodesToProcess.back();
+NodesToProcess.pop_back();
+
+if (const auto *CS = Node.get())
+  R = CS;
+if (Node.get())
+  break;
+Parents = Ctx.getParents(Node);
+NodesToProcess.append(Parents.begin(), Parents.end());
+  }
+  return R;
+}
+  };
+
+  Visitor V{Literals, Result, Ctx};
+  V.TraverseDecl(D);
+}
+
 bool RemoveUsingNamespace::prepare(const Selection ) {
   // Find the 'using namespace' directive under the cursor.
   auto *CA = Inputs.ASTSelection.commonAncestor();
@@ -144,7 +205,15 @@
   // Collect all references to symbols from the namespace for which we're
   // removing the directive.
   std::vector IdentsToQualify;
+  // Collect all user-defined literals from the namespace for which we're
+  // removing the directive
+  std::map> LiteralsToUsing;
+
   

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

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

[PATCH] D138300: [clangd] Support type hints for `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

@nridge Thank you for reviewing!

If this patch is ready to land, could you please help me commit it? Thanks 
again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138300

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

[PATCH] D137817: [clangd] Improve action `RemoveUsingNamespace` for user-defined literals

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

I think this patch is ready for another review now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137817

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

[PATCH] D137817: [clangd] Improve action `RemoveUsingNamespace` for user-defined literals

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

I think this patch is ready for another review now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137817

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

[PATCH] D137817: [clangd] Improve action `RemoveUsingNamespace` for user-defined literals

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 483749.
v1nh1shungry added a comment.

Note:
Currently if there are user-defined literals, this patch will find them and get 
their
`CompoundStmt` parents. If there is one, it will record the parent so that it 
can add the
using-declaration in it later. If there is not, it means the user-defined 
literal is in
the top-level, and it is reasonable to do nothing for it. I choose 
`CompoundStmt` because
it covers many cases, like blocks, functions, lambdas.

Since I failed to find a way to use only `ReferenceLoc` to get the parents, I 
have to
traverse the whole `Decl` again to find out the `DeclRefExpr` and then use 
`ASTContext`
to get its parents. Also this implementation can cause redundance in some cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137817

Files:
  clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
  clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
@@ -250,20 +250,33 @@
 foo + 10;
   }
 )cpp"},
-  {// Does not qualify user-defined literals
-   R"cpp(
-  namespace ns {
-  long double operator "" _w(long double);
+  {
+  R"cpp(
+  namespace a {
+  long double operator""_a(long double);
+  inline namespace b {
+  long double operator""_b(long double);
+  } // namespace b
+  } // namespace a
+  using namespace ^a;
+  int main() {
+1.0_a;
+1.0_b;
   }
-  using namespace n^s;
-  int main() { 1.5_w; }
 )cpp",
-   R"cpp(
-  namespace ns {
-  long double operator "" _w(long double);
-  }
+  R"cpp(
+  namespace a {
+  long double operator""_a(long double);
+  inline namespace b {
+  long double operator""_b(long double);
+  } // namespace b
+  } // namespace a
   
-  int main() { 1.5_w; }
+  int main() {using a::operator""_a;
+using a::operator""_b;
+1.0_a;
+1.0_b;
+  }
 )cpp"}};
   for (auto C : Cases)
 EXPECT_EQ(C.second, apply(C.first)) << C.first;
Index: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
@@ -13,6 +13,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -101,6 +102,62 @@
   return D;
 }
 
+using UserDefinedLiteralSet = llvm::SmallPtrSet;
+
+void handleUserDefinedLiterals(
+Decl *D, const UserDefinedLiteralSet ,
+std::map , ASTContext ) {
+  struct Visitor : RecursiveASTVisitor {
+Visitor(const UserDefinedLiteralSet ,
+std::map ,
+ASTContext )
+: Literals{Literals}, Result{Result}, Ctx{Ctx} {}
+
+const UserDefinedLiteralSet 
+std::map 
+ASTContext 
+
+bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+  const NamedDecl *ND = DRE->getFoundDecl();
+  // If DRE references to a user-defined literal
+  if (ND->getDeclName().getNameKind() ==
+  DeclarationName::CXXLiteralOperatorName) {
+for (const NamedDecl *Literal : Literals) {
+  if (ND == Literal) {
+// If the user-defined literal locates in a CompoundStmt,
+// we mark the CompoundStmt so that we can add the using-declaration
+// in it later. If not, the user-defined literal is used in the
+// top-level, it is reasonable to do nothing for it.
+if (const CompoundStmt *CS = getCompoundParent(DRE))
+  Result[CS->getLBracLoc()].insert(ND);
+return true;
+  }
+}
+  }
+  return true;
+}
+
+// Get the closet CompoundStmt parent of DRE
+const CompoundStmt *getCompoundParent(DeclRefExpr *DRE) {
+  DynTypedNodeList Parents = Ctx.getParents(*DRE);
+  llvm::SmallVector NodesToProcess{Parents.begin(),
+Parents.end()};
+  while (!NodesToProcess.empty()) {
+DynTypedNode Node = NodesToProcess.back();
+NodesToProcess.pop_back();
+if (const auto *CS = Node.get())
+  return CS;
+Parents = Ctx.getParents(Node);
+NodesToProcess.append(Parents.begin(), Parents.end());
+  }
+  return nullptr;
+}
+  };
+
+  Visitor V{Literals, Result, 

[PATCH] D138300: [clangd] Support type hints for `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 483720.
v1nh1shungry added a comment.

reimplement and address review's suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138300

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1361,6 +1361,29 @@
   EXPECT_THAT(hintsOfKind(AST, InlayHintKind::Type), IsEmpty());
 }
 
+TEST(TypeHints, Decltype) {
+  assertTypeHints(R"cpp(
+$a[[decltype(0)]] a;
+// FIXME: will be nice to show `: int` instead
+$b[[decltype(a)]] b;
+const $c[[decltype(0)]]  = b;
+
+// Don't show for dependent type
+template 
+constexpr decltype(T{}) d;
+
+$e[[decltype(0)]] e();
+auto f() -> $f[[decltype(0)]];
+
+template  struct Foo;
+using G = Foo<$g[[decltype(0)]], float>;
+  )cpp",
+  ExpectedHint{": int", "a"},
+  ExpectedHint{": decltype(0)", "b"},
+  ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
+  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"});
+}
+
 TEST(DesignatorHints, Basic) {
   assertDesignatorHints(R"cpp(
 struct S { int x, y, z; };
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -218,6 +218,13 @@
 StructuredBindingPolicy.PrintCanonicalTypes = true;
   }
 
+  bool VisitTypeLoc(TypeLoc TL) {
+if (const auto *DT = llvm::dyn_cast(TL.getType()))
+  if (QualType UT = DT->getUnderlyingType(); !UT->isDependentType())
+addTypeHint(TL.getSourceRange(), UT, ": ");
+return true;
+  }
+
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
 // Weed out constructor calls that don't look like a function call with
 // an argument list, by checking the validity of getParenOrBraceRange().


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1361,6 +1361,29 @@
   EXPECT_THAT(hintsOfKind(AST, InlayHintKind::Type), IsEmpty());
 }
 
+TEST(TypeHints, Decltype) {
+  assertTypeHints(R"cpp(
+$a[[decltype(0)]] a;
+// FIXME: will be nice to show `: int` instead
+$b[[decltype(a)]] b;
+const $c[[decltype(0)]]  = b;
+
+// Don't show for dependent type
+template 
+constexpr decltype(T{}) d;
+
+$e[[decltype(0)]] e();
+auto f() -> $f[[decltype(0)]];
+
+template  struct Foo;
+using G = Foo<$g[[decltype(0)]], float>;
+  )cpp",
+  ExpectedHint{": int", "a"},
+  ExpectedHint{": decltype(0)", "b"},
+  ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
+  ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"});
+}
+
 TEST(DesignatorHints, Basic) {
   assertDesignatorHints(R"cpp(
 struct S { int x, y, z; };
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -218,6 +218,13 @@
 StructuredBindingPolicy.PrintCanonicalTypes = true;
   }
 
+  bool VisitTypeLoc(TypeLoc TL) {
+if (const auto *DT = llvm::dyn_cast(TL.getType()))
+  if (QualType UT = DT->getUnderlyingType(); !UT->isDependentType())
+addTypeHint(TL.getSourceRange(), UT, ": ");
+return true;
+  }
+
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
 // Weed out constructor calls that don't look like a function call with
 // an argument list, by checking the validity of getParenOrBraceRange().
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138300: [clangd] Support type hints for `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry reclaimed this revision.
v1nh1shungry added a comment.

Got it! Thank you for the detailed explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138300

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

[PATCH] D138300: [clangd] Support type hints for `decltype(expr)`

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thanks for the feedback!

> One high-level thought I had is: what if we attached the type hint to the 
> closing `)` of the decltype (and had it pertain only to the `decltype(expr)`, 
> not anything surrounding it like `const` or `&`)? It seems to me that this 
> would both simplify the implementation, and allow us to show hints in places 
> where `decltype` is used in a context unrelated to a variable or function 
> declaration (for example, in something like `using Foo = A C>, D>`, if `decltype(expr)` was `int`, we would show `using Foo = 
> A, D>`. What do you think about this approach?

I think this approach is better.

> I think showing type hints for `decltype(expr)` would be a nice enhancement. 
> @v1nh1shungry, are you interested in working further on this?

Sure!




Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1155
+// FIXME: Nice to show `: int &`
+decltype((i)) $a[[a]] = i;
+  )cpp",

The only concern I have is how to deal with this situation.

When I wrote this patch I thought maybe I could remove the `&` and figure out 
the underlying type of `decltype(0)` recursively and then add the `&` back. But 
at that time I couldn't find a way to get the referenced type or the pointee 
type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138300

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

[PATCH] D137817: [clangd] Improve action `RemoveUsingNamespace` for user-defined literals

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 483582.
v1nh1shungry added a comment.

Reimplemented.

Note:

Failed to find a way to use `ReferenceLoc` to insert the using-declarations.
Currently I have to visit the `Decl` again and look for `CompoundStmt`, if there
is any target user-defined literal in the `CompoundStmt` I mark it so that
I can add the using-declaration in it later.

In this way this implementation covers many cases, and does nothing for 
user-defined
literals used in the top-level. But it can cause redundancy and is too complex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137817

Files:
  clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
  clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
@@ -250,20 +250,33 @@
 foo + 10;
   }
 )cpp"},
-  {// Does not qualify user-defined literals
-   R"cpp(
-  namespace ns {
-  long double operator "" _w(long double);
+  {
+  R"cpp(
+  namespace a {
+  long double operator""_a(long double);
+  inline namespace b {
+  long double operator""_b(long double);
+  } // namespace b
+  } // namespace a
+  using namespace ^a;
+  int main() {
+1.0_a;
+1.0_b;
   }
-  using namespace n^s;
-  int main() { 1.5_w; }
 )cpp",
-   R"cpp(
-  namespace ns {
-  long double operator "" _w(long double);
-  }
+  R"cpp(
+  namespace a {
+  long double operator""_a(long double);
+  inline namespace b {
+  long double operator""_b(long double);
+  } // namespace b
+  } // namespace a
   
-  int main() { 1.5_w; }
+  int main() {using a::operator""_a;
+using a::operator""_b;
+1.0_a;
+1.0_b;
+  }
 )cpp"}};
   for (auto C : Cases)
 EXPECT_EQ(C.second, apply(C.first)) << C.first;
Index: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
@@ -101,6 +101,60 @@
   return D;
 }
 
+using UserDefinedLiteralSet = llvm::SmallPtrSet;
+
+UserDefinedLiteralSet findLiterals(Stmt *S,
+   const UserDefinedLiteralSet ) {
+  struct Visitor : RecursiveASTVisitor {
+Visitor(const UserDefinedLiteralSet ) : Targets{Targets} {}
+
+const UserDefinedLiteralSet 
+UserDefinedLiteralSet Result;
+
+bool VisitDeclRefExpr(const DeclRefExpr *E) {
+  const auto *D = E->getFoundDecl();
+  if (D->getDeclName().getNameKind() ==
+  DeclarationName::CXXLiteralOperatorName) {
+for (const auto *Target : Targets) {
+  if (D == Target) {
+Result.insert(D);
+return true;
+  }
+}
+  }
+  return true;
+}
+  };
+
+  Visitor V{Targets};
+  V.TraverseStmt(S);
+  return V.Result;
+}
+
+void handleUserDefinedLiterals(
+Decl *D, const UserDefinedLiteralSet ,
+std::map ) {
+  struct Visitor : RecursiveASTVisitor {
+Visitor(const UserDefinedLiteralSet ,
+std::map )
+: Literals{Literals}, Result{Result} {}
+
+const UserDefinedLiteralSet 
+std::map 
+
+bool VisitCompoundStmt(CompoundStmt *CS) {
+  auto LiteralsFound = findLiterals(CS, Literals);
+  if (!LiteralsFound.empty())
+Result[CS->getBeginLoc()].insert(LiteralsFound.begin(),
+ LiteralsFound.end());
+  return true;
+}
+  };
+
+  Visitor V{Literals, Result};
+  V.TraverseDecl(D);
+}
+
 bool RemoveUsingNamespace::prepare(const Selection ) {
   // Find the 'using namespace' directive under the cursor.
   auto *CA = Inputs.ASTSelection.commonAncestor();
@@ -144,7 +198,15 @@
   // Collect all references to symbols from the namespace for which we're
   // removing the directive.
   std::vector IdentsToQualify;
+  // Collect all user-defined literals from the namespace for which we're
+  // removing the directive
+  std::map LiteralsToUsing;
+
   for (auto  : Inputs.AST->getLocalTopLevelDecls()) {
+if (SM.isBeforeInTranslationUnit(D->getBeginLoc(), FirstUsingDirectiveLoc))
+  continue; // Directive was not visible before this point.
+
+UserDefinedLiteralSet Literals;
 findExplicitReferences(
 D,
 [&](ReferenceLoc Ref) {
@@ -164,15 +226,20 @@
 // Avoid adding qualifiers before user-defined literals, e.g.
 //   using namespace std;
 //   auto s = "foo"s; // Must 

[PATCH] D137817: [clangd] Improve action `RemoveUsingNamespace` for user-defined literals

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 483114.
v1nh1shungry added a comment.

apply review's suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137817

Files:
  clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
  clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp
@@ -250,20 +250,33 @@
 foo + 10;
   }
 )cpp"},
-  {// Does not qualify user-defined literals
-   R"cpp(
-  namespace ns {
-  long double operator "" _w(long double);
+  {
+  R"cpp(
+  namespace a {
+  long double operator""_a(long double);
+  inline namespace b {
+  long double operator""_b(long double);
+  } // namespace b
+  } // namespace a
+  using namespace ^a;
+  int main() {
+1.0_a;
+1.0_b;
   }
-  using namespace n^s;
-  int main() { 1.5_w; }
 )cpp",
-   R"cpp(
-  namespace ns {
-  long double operator "" _w(long double);
-  }
+  R"cpp(
+  namespace a {
+  long double operator""_a(long double);
+  inline namespace b {
+  long double operator""_b(long double);
+  } // namespace b
+  } // namespace a
   
-  int main() { 1.5_w; }
+  int main() {using a::operator""_a;
+using a::operator""_b;
+1.0_a;
+1.0_b;
+  }
 )cpp"}};
   for (auto C : Cases)
 EXPECT_EQ(C.second, apply(C.first)) << C.first;
Index: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
@@ -144,6 +144,9 @@
   // Collect all references to symbols from the namespace for which we're
   // removing the directive.
   std::vector IdentsToQualify;
+  // Collect all using-declarations to add for user-defined literals declared
+  // in the namespace for which we're removing the directive
+  std::map> DeclsToAdd;
   for (auto  : Inputs.AST->getLocalTopLevelDecls()) {
 findExplicitReferences(
 D,
@@ -164,15 +167,24 @@
 // Avoid adding qualifiers before user-defined literals, e.g.
 //   using namespace std;
 //   auto s = "foo"s; // Must not changed to auto s = "foo" std::s;
-// FIXME: Add a using-directive for user-defined literals
-// declared in an inline namespace, e.g.
-//   using namespace s^td;
-//   int main() { cout << "foo"s; }
-// change to
-//   using namespace std::literals;
-//   int main() { std::cout << "foo"s; }
-if (Kind == DeclarationName::NameKind::CXXLiteralOperatorName)
+// Also add a using-declaration to keep codes well-formed, e.g.
+//   using namespace std;
+//   int main() {
+// auto s = "foo"s;
+//   }
+// will change to
+//   int main() {
+// using std::operator""_s;
+// auto s = "foo"s;
+//   }
+// FIXME: Currently this only works for functions and methods
+//declared right in the top-level
+if (Kind == DeclarationName::NameKind::CXXLiteralOperatorName) {
+  if (const Stmt *Body = D->getBody())
+DeclsToAdd[Body->getBeginLoc()].insert(
+T->getQualifiedNameAsString());
   return;
+}
   }
   SourceLocation Loc = Ref.NameLoc;
   if (Loc.isMacroID()) {
@@ -217,6 +229,16 @@
   /*Length=*/0, Qualifier)))
   return std::move(Err);
   }
+  // Produce replacements to add the using-declarations
+  for (const auto &[Loc, Decls] : DeclsToAdd) {
+std::string UsingDecls;
+for (const auto  : Decls)
+  UsingDecls += llvm::formatv("using {0};\n", D.getKey());
+UsingDecls.pop_back();
+if (auto Err = R.add(
+tooling::Replacement(SM, Loc.getLocWithOffset(1), 0, UsingDecls)))
+  return std::move(Err);
+  }
   return Effect::mainFileEdit(SM, std::move(R));
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits