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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
