[PATCH] D83814: [clangd] Add Random Forest runtime for code completion.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 added inline comments.


================
Comment at: clang-tools-extra/clangd/quality/CompletionModelCodegen.py:39
+    '''Returns the header guard for the generated header.'''
+    return "GENERATED_DECISION_FOREST_MODEL_{}_H".format(filename.upper())
+
----------------
adamcz wrote:
> Why GENERATED_DECISON_FOREST_MODEL instead of output_dir, to be consistent with header guards for other files? Doesn't matter much for generated code, but if someone opens this in vim they'll see warnings.
The output_dir is the absolute path and not a project relative path.
I tried to stick with a special prefix for header guard as done in other Generated headers (e.g. protos)

If someone opens this in vim, there would many other warnings that they would see like "unused_label" ;)
I don't think that would be a concern since it would be opened for inspection and not for editing.


================
Comment at: clang-tools-extra/clangd/quality/CompletionModelCodegen.py:177
+
+    code += "  friend float Evaluate(const {}&);\n".format(cpp_class.name)
+    code += "};\n"
----------------
adamcz wrote:
> Is there a reason to make this a friend free-function instead of static method on the Example class? The problem is that you now end up with clang::clangd::Evaluate, so if we every re-use this code gen for another model we'll have a name collision.
The class name ("Example" currently) would be different for a different model and therefore there would be another overload for `Evaluate(const AnotherClass&)` even if the namespaces are same (`clang::clangd`).


================
Comment at: clang-tools-extra/clangd/unittests/model/CategoricalFeature.h:1
+namespace ns1 {
+namespace ns2 {
----------------
adamcz wrote:
> Can we rename this directory? quality/model makes some sense (although it would be better to include something about code completion there), but unittests/model is not very descriptive - what model?
> 
> How about unittests/decision_forest_model/ or something like that? Or go with the Input/TEST_NAME pattern.
You are right "quality" wasn't indicative of code completion here but we decided to be consistent with the current naming. The current heuristics for the ranking are in Quality.h and Quality.cpp ;-)

changed the dir name in unittests.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83814