[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/CMakeLists.txt:30
+  
+set(DF_COMPILER ${CMAKE_CURRENT_SOURCE_DIR}/CompletionModelCodegen.py)
+include(${CMAKE_CURRENT_SOURCE_DIR}/CompletionModel.cmake)
----------------
sammccall wrote:
> if you want the compiler script to be a parameter, make it an argument to the function rather than a magic variable. 
> 
> But really, I think the CompletionModel.cmake is tightly coupled to the python script, I think it can be hardcoded there.
Hardcoded it in .cmake file.


================
Comment at: clang-tools-extra/clangd/CompletionModel.cmake:1
+# Run the Completion Model Codegenerator on the model in the 
+# ${model} directory.
----------------
sammccall wrote:
> I think there's some confusion in the naming.
> 
> You've got the code split into two locations: the generic generator and the specific code completion model.
> 
> However the directory named just "model" contains the specific stuff, and the generic parts are named "completionmodel!".
> 
> I'd suggest either:
>  - don't generalize, and put everything in clangd/quality or so
>  - split into clangd/quality/ and clangd/forest/ for the specific/generic parts
Moved .cmake, codegen and model in quality dir.


================
Comment at: clang-tools-extra/clangd/CompletionModel.cmake:5
+# ${CMAKE_BINARY_DIR}/generated/decision_forest. The generated header
+# will define a C++ class called ${cpp_class} - which may be a
+# namespace-qualified class name.
----------------
sammccall wrote:
> what does the class do?
The class specifies the name and scope of the Feature class. `clang::clangd::Example` in this case.


================
Comment at: clang-tools-extra/clangd/CompletionModel.cmake:17
+    COMMAND "${Python3_EXECUTABLE}" ${DF_COMPILER} 
+      --model ${model}
+      --output_dir ${output_dir}
----------------
sammccall wrote:
> I'd suggest passing the component filenames explicitly here since you're computing them anyway
This allows the generated cc file to include the header using "filename.h".
If we give the filepath as input, we would have to strip out the filename from it. 

Although I like the current notion of being explicit that the output_dir contains the two files. We need to add output_dir to include path to use this library.


================
Comment at: clang-tools-extra/clangd/CompletionModel.cmake:29
+    GENERATED 1
+    COMPILE_FLAGS -Wno-unused-label)
+
----------------
sammccall wrote:
> this needs to be guarded based on the compiler - other compilers use different flags
> 
> I'd suggest just -Wno-usuned
only MSVC needed a different flag. `-Wno-unused` works with Clang and GCC.
https://godbolt.org/z/Gvdne7



================
Comment at: clang-tools-extra/clangd/CompletionModel.cmake:31
+
+  set(GENERATED_CC ${df_cpp} PARENT_SCOPE)
+  set(DF_INCLUDE ${output_dir} PARENT_SCOPE)  
----------------
sammccall wrote:
> It'd be nice to avoid passing data out by setting magic variables with generic names.
> 
> The caller is already passing in the filename they want, so they know what it is.
We can avoid `GENERATED_CC`. But I still wanted to keep the output directory as a detail in this function itself and not as an input parameter.
Changed the name to more specific name `DECISION_FOREST_OUTPUT_DIR`. 



================
Comment at: clang-tools-extra/clangd/CompletionModelCodegen.py:9
+
+class Feature:
+    class Type(Enum):
----------------
sammccall wrote:
> Hmm, do these classes really pay for themselves compared to just using the JSON-decoded data structures directly and writing functions?
> 
> e.g.
> 
> ```
> def setter(feature):
>   if (feature['kind'] == KIND_CATEGORICAL)
>     return "void Set{feature}(float V) {{ {feature} = OrderEncode(V); }}".format(feature['name'])
>   ...
> ```
Removed the Feature class and Tree.
CppClass calculates and holds the namespaces which I felt convenient. 


================
Comment at: clang-tools-extra/clangd/CompletionModelCodegen.py:52
+        self.fname = fname
+        assert not cpp_class.startswith(
+            "::"), "Do not fully qualify class name."
----------------
sammccall wrote:
> why not assert this on self.ns so that "::Foo" will work fine?
Allowed fully qualified names of classes.


================
Comment at: clang-tools-extra/clangd/CompletionModelCodegen.py:70
+        return "GENERATED_CODE_COMPLETION_MODEL_{}_H".format(
+            reduce(lambda x, y: x + ('_' if y.isupper() else '') + y,
+                   self.fname).upper())
----------------
sammccall wrote:
> `''.join('_' if x in '-' else x.upper() for x in self.fname)` ?
Sorry for making this complicated. filename was assumed to be in PascalCase (and not contain '-' at all).
I wanted to convert it to UPPER_SNAKE_CASE. To avoid unnecessary complexity, lets simply convert it to upper case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83814