kadircet added a comment.

(sorry for forgetting about this)



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:450
+        if (Snippet->front() == '<')
+          return Snippet->substr(0, Snippet->find('('));
+        return "";
----------------
what if we have `(` in the template parameter list ? i think we need to 
literally find the matching `>` and include all the text in between instead.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:456
+      // template argument list but it would be complicated.
+      if (NextTokenKind == tok::less && Snippet->front() == '<')
+        return "";
----------------
nit: again might be worth putting the easy-to-reason case above to reduce 
mental load when reading.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:459
+    }
     if (!Snippet)
       // All bundles are function calls.
----------------
nit: move this case above and get rid of `if(Snippet` part of the check?


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2982
   )cpp";
+  // FIXME(kirillbobyrev): Also strip prefix of identifier token before the
+  // cursor from completion item ("fo" in this case).
----------------
umm, where did this fixme come from exactly? don't we emit these as 
replacements for the `fo` part of the text anyway ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89870

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

Reply via email to