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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added a comment.

Looks good to me overall, some minor style comments included ;-)

Do we expect this to be a generic model code generator that gets reused for other things? If not, maybe we can hardcode more (like the namespace, class name, etc), but if you think there's other use cases for this then this LGTM.



================
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())
+
----------------
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.


================
Comment at: clang-tools-extra/clangd/quality/CompletionModelCodegen.py:57
+    Control falls through if condition is evaluated to false."""
+    return "{label}: if(E.{feature} >= {encoded} /*{threshold}*/) goto {true_label};".format(
+        label=label,
----------------
nit: add space after if for readability (also below)


================
Comment at: clang-tools-extra/clangd/quality/CompletionModelCodegen.py:93
+def tree(t, tree_num: int, node_num: int):
+    """Returns code for inferencing a Decision Tree.
+
----------------
Please extend the comment to mention the second return value (size of the tree)


================
Comment at: clang-tools-extra/clangd/quality/CompletionModelCodegen.py:105
+    """
+    label = "t{tree}_n{node}".format(tree=tree_num, node=node_num)
+    code = []
----------------
This is a good place to use an Python's f-string.

Also in few places below.


================
Comment at: clang-tools-extra/clangd/quality/CompletionModelCodegen.py:126
+
+    return code + false_code + true_code, 1 + false_size+true_size
+
----------------
style nit: be consistent with spaces around +


================
Comment at: clang-tools-extra/clangd/quality/CompletionModelCodegen.py:177
+
+    code += "  friend float Evaluate(const {}&);\n".format(cpp_class.name)
+    code += "};\n"
----------------
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.


================
Comment at: clang-tools-extra/clangd/quality/CompletionModelCodegen.py:271
+    parser = argparse.ArgumentParser('DecisionForestCodegen')
+    parser.add_argument('--filename', help='output file name.')
+    parser.add_argument('--output_dir', help='output directory')
----------------
nit: be consistent about putting a "." at the end of the help text or not.


================
Comment at: clang-tools-extra/clangd/unittests/model/CategoricalFeature.h:1
+namespace ns1 {
+namespace ns2 {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83814