This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Indent after breaking Javadoc annotated line
ClosedPublic

Authored by krasimir on Jul 25 2018, 7:13 AM.

Diff Detail

Event Timeline

krasimir created this revision.Jul 25 2018, 7:13 AM
krasimir updated this revision to Diff 157452.Jul 26 2018, 3:50 AM
  • Update tests
mprobst added inline comments.Jul 26 2018, 5:22 AM
lib/Format/BreakableToken.cpp
526

Shouldn't this check the set above?

unittests/Format/FormatTestComments.cpp
3109

add a test using @return or @type?

3154

do we already have a test that @param {oh noes this is really long} foo isn't broken?

mprobst added inline comments.Jul 27 2018, 2:10 AM
unittests/Format/FormatTestComments.cpp
3109

as discussed, make sure to test for LK_JavaScript, because that affects this with comment pragmas (that we should probably disable).

krasimir updated this revision to Diff 157681.Jul 27 2018, 7:03 AM
krasimir marked 3 inline comments as done.
  • Address review comments
krasimir marked an inline comment as done.Jul 27 2018, 7:03 AM
krasimir added inline comments.
lib/Format/BreakableToken.cpp
526

ahahah, sure!

unittests/Format/FormatTestComments.cpp
3154

As discussed offline, we'll gonna go with breaking that.

krasimir marked an inline comment as done.Jul 27 2018, 7:04 AM
mprobst accepted this revision.Jul 27 2018, 7:13 AM
mprobst added inline comments.
lib/Format/Format.cpp
814

@exports isn't really a thing in JS, it is really @export.

But more generally, where do you get stuff like @module from, and what is it supposed to do?

google3 has @pintomodule, @mods and @modName, but I don't think those need special treatment.

I think I'd drop everything except the @see here.

This revision is now accepted and ready to land.Jul 27 2018, 7:13 AM
krasimir added inline comments.Jul 27 2018, 8:36 AM
lib/Format/Format.cpp
814

There were existing tests having these.
I can find a reference to @exports and @module here: http://usejsdoc.org/tags-exports.html.
I think that in general if the annotation is supposed to only have the form @tag some-long-path-separated-machine-readable-identifier, that should be left on a single line.
For @mods, I vaguely remember adding that test case while fixing a bug that it shouldn't be broken.
Still, happy to remove these and we'll wait to see if anyone complains.

mprobst added inline comments.Jul 27 2018, 11:29 PM
lib/Format/Format.cpp
814

Please don't change the test code that uses @export, that's a commonly used Closure Compiler annotation.

JSDoc isn't really a consistently applied standard, over time implementations have diverged a lot, so I'm afraid those docs might not apply to Closure Compiler's use of JSDoc. I don't think this hurts here, but I wouldn't expect anybody getting much use out of this either, so it might be cleaner to drop.

krasimir updated this revision to Diff 157925.Jul 30 2018, 1:28 AM
  • Address comments
krasimir marked 3 inline comments as done.Jul 30 2018, 1:32 AM
This revision was automatically updated to reflect the committed changes.