[clang-tools-extra] [clang] [clangd] Prevent printing huge initializer lists in hover definitions (PR #79746)

[kadir çetinkaya via cfe-commits]

https://github.com/kadircet requested changes to this pull request.


https://github.com/llvm/llvm-project/pull/79746
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang] [clangd] Prevent printing huge initializer lists in hover definitions (PR #79746)

[kadir çetinkaya via cfe-commits]

@@ -1720,6 +1720,12 @@ void StmtPrinter::VisitInitListExpr(InitListExpr* Node) {
   OS << "{";
   for (unsigned i = 0, e = Node->getNumInits(); i != e; ++i) {
 if (i) OS << ", ";
+// TODO: There is duplicated functionality in APValue::printPretty.
+// Would be good to consolidate the two.
+if (!Policy.EntireContentsOfLargeArray && i == 10) {

kadircet wrote:

i am not sure if usage of this flag is actually appropriate here. it 
specifically says "contents of large arrays" but here we have an initializer 
list, not necessarily an array.

moreover this fix is in clang, independent of clangd, so I think it deserves 
its own patch, with relevant owners from clang. can you separate it out? i'd 
like to hear their opinion on using this flag.

https://github.com/llvm/llvm-project/pull/79746
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang] [clangd] Prevent printing huge initializer lists in hover definitions (PR #79746)

[Raoul Wols via cfe-commits]

@@ -1720,6 +1720,12 @@ void StmtPrinter::VisitInitListExpr(InitListExpr* Node) {
   OS << "{";
   for (unsigned i = 0, e = Node->getNumInits(); i != e; ++i) {
 if (i) OS << ", ";
+// TODO: There is duplicated functionality in APValue::printPretty.
+// Would be good to consolidate the two.
+if (!Policy.EntireContentsOfLargeArray && i == 10) {

rwols wrote:

I was also kind of unsure about the usage of `EntirecontentsOfLargeArray` here. 
I feel like StmtPrinter should always output valid C++ code  but that's not 
possible using `EntireContentsOfLargeArray`.

https://github.com/llvm/llvm-project/pull/79746
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang] [clangd] Prevent printing huge initializer lists in hover definitions (PR #79746)

[Raoul Wols via cfe-commits]

@@ -138,15 +138,9 @@ std::string getNamespaceScope(const Decl *D) {
 
 std::string printDefinition(const Decl *D, PrintingPolicy PP,
 const syntax::TokenBuffer &TB) {
-  if (auto *VD = llvm::dyn_cast(D)) {
-if (auto *IE = VD->getInit()) {
-  // Initializers might be huge and result in lots of memory allocations in
-  // some catostrophic cases. Such long lists are not useful in hover cards
-  // anyway.
-  if (200 < TB.expandedTokens(IE->getSourceRange()).size())
-PP.SuppressInitializers = true;
-}
-  }
+  // Initializers might be huge and result in lots of memory allocations in 
some
+  // catostrophic cases. Such long lists are not useful in hover cards anyway.
+  PP.EntireContentsOfLargeArray = false;

rwols wrote:

> a more concrete approach could be based on counting number of "total" 
> elements in the initializer list, and setting the suppression flag based on 
> that. i'd actually lean towards such a solution, WDYT?

I agree, but don't know where to start on this. Since TokenBuffer doesn't have 
tokens from the preamble, what alternatives are there?

https://github.com/llvm/llvm-project/pull/79746
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang] [clangd] Prevent printing huge initializer lists in hover definitions (PR #79746)

[kadir çetinkaya via cfe-commits]

@@ -138,15 +138,9 @@ std::string getNamespaceScope(const Decl *D) {
 
 std::string printDefinition(const Decl *D, PrintingPolicy PP,
 const syntax::TokenBuffer &TB) {
-  if (auto *VD = llvm::dyn_cast(D)) {
-if (auto *IE = VD->getInit()) {
-  // Initializers might be huge and result in lots of memory allocations in
-  // some catostrophic cases. Such long lists are not useful in hover cards
-  // anyway.
-  if (200 < TB.expandedTokens(IE->getSourceRange()).size())
-PP.SuppressInitializers = true;
-}
-  }
+  // Initializers might be huge and result in lots of memory allocations in 
some
+  // catostrophic cases. Such long lists are not useful in hover cards anyway.
+  PP.EntireContentsOfLargeArray = false;

kadircet wrote:

I wasn't suggesting to use tokens, but rather count the number of elements 
directly through the AST.
we've got the expression available already, so we can go over its `chidlren()` 
and count total elements we **might** encounter, and suppress intializers based 
on that (when the tokens aren't available)

https://github.com/llvm/llvm-project/pull/79746
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits