sammccall added a comment.
Thanks for doing this! Some high-level comments here, will review
implementation next:
**There seems to be a lot of "decoration" on the hint text.**
If declaring a class "Foo" then we add ` /* class ` before and ` */ ` after
Foo, for a total of 14 characters apart from the name itself. Even if we're
mostly adding these hints to mostly-empty lines, I think we should try to be a
bit more economical to reduce breaking up the flow of the code.
We could reduce this by some combination of:
- not using comment syntax, e.g. ` class Foo` or `(class Foo)`. We've used
pseudo-C++ syntax for other hints, but largely because it was convenient and
terse.
- using `// ` rather than `/* */` comment syntax where possible, e.g. `//
class Foo`
- dropping the kind, e.g. `/* Foo */`
- dropping the name, e.g. `/* class */`
I think my favorite of these is probably `// class Foo` - this is 5 characters
shorter than the current version, provides all the information, uses pseudo-C++
syntax, is consistent with clang-format's namespace comments, and would be
suitable for a "insert closing comment" code action in future.
This implies putting the hints after the semicolon (I think it's fine to do
this purely textually by examining characters), and choosing some behavior when
there's a trailing comment or code (my suggestion would be just to drop the
hint in this case).
WDYT?
**Naming**: `EndDefinitionComment` is a bit of a mouthful, I'd like something
simpler.
"Comment" is the form of the hint, not the thing being hinted, so it seems
superfluous. ("Designator" is similar, but I couldn't think of a good
alternative there).
"Definition" is OK but a little vague - many things have definitions, so it's
not clear what we'll hint. I think "Block" is clearer. It could describe e.g.
the end of a long `while` loop, but I think that's actually OK - we could add
that one day, and I think it would make sense to have it controlled by the same
setting.
So I think my favorite name would be `EndBlock` - most critically in the
configuration, but also consistently in the code.
(Renaming things is a lot of churn, feel free to defer changes until we've got
a solid agreement on a name!)
**Configurability**: Does the min-lines really need a config knob now?
Fairly few people change the defaults, so:
- we'll need to find the best default in any case, and
- to justify adding an option it should be *very* useful to the people that use
it
In particular, "the default is arbitrary" is not itself a strong reason to make
it configurable.
The downsides to configuration are extra testing/plumbing, and we also lock
ourselves into "number of lines" being the criterion - maybe we want a
different heuristic in future and this will interfere.
If we ship without it, we'll get clear feedback on whether the default is good
and if customization is needed, and it's easy to add an option later.
Unless I'm missing something, I'd prefer this to be a constant in the code for
now.
I think the default of 2 you've got here is worth a try, maybe after some
experience a higher number will be better.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150635/new/
https://reviews.llvm.org/D150635
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
