Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2139883002:
Add clang-tidy documentation (Closed)
DescriptionAdd clang-tidy documentation
Add documentation for how to use clang-tidy in chromium.
BUG=None
Committed: https://crrev.com/be2898eb2f8115fcee7a8cb9de17eff8cc6ee661
Cr-Commit-Position: refs/heads/master@{#404872}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add scary warning #
Total comments: 10
Patch Set 3 : Nits #Messages
Total messages: 13 (5 generated)
rdevlin.cronin@chromium.org changed reviewers: + thakis@chromium.org
Heya Nico, mind taking a look? https://codereview.chromium.org/2139883002/diff/1/docs/clang_tidy.md File docs/clang_tidy.md (right): https://codereview.chromium.org/2139883002/diff/1/docs/clang_tidy.md#newcode1 docs/clang_tidy.md:1: # Clang Tidy Not all markdown viewers are created equal - do you know if there's a way to test this in a gitiles-like viewer? Alternatively, we can commit it, view it there for realz, and update as needed. https://codereview.chromium.org/2139883002/diff/1/docs/clang_tidy.md#newcode89 docs/clang_tidy.md:89: Questions? Reach out to rdevlin.cronin@chromium.org or thakis@chromium.org. I'm volunteering you here. Let me know if you want out now. :)
I'd make this text a lot more cautious ("this is very experimental; we're
working on making this easy to use; for now it's only for people who like
tinkering with things. with that out of the way, here's how you could use it").
As I mentioned somewhere, etienneb is working on making this easier to use, and
there are plans for running it automatically with results either in codesearch
or rietveld/gerrit. Documenting this seems better than not documenting it, but I
don't want the text to sell it too much as it's pretty cumbersome to use atm.
On 2016/07/12 01:08:18, Nico wrote:
> I'd make this text a lot more cautious ("this is very experimental; we're
> working on making this easy to use; for now it's only for people who like
> tinkering with things. with that out of the way, here's how you could use
it").
> As I mentioned somewhere, etienneb is working on making this easier to use,
and
> there are plans for running it automatically with results either in codesearch
> or rietveld/gerrit. Documenting this seems better than not documenting it, but
I
> don't want the text to sell it too much as it's pretty cumbersome to use atm.
Fair point. Added a section for that.
lgtm, some optional comments below https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md File docs/clang_tidy.md (right): https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:18: provides a convenient interface for writing new checks. i'd omit the last sentence https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:53: ninja -C out/release chrome upper-case R, then it works with gyp too https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:57: ninja -C out/release -t compdb objxx cxx > compile_commands.json ditto. (also, isn't it "objcxx" for mm files? not sure) https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:67: # Set the clang-tidy binary path, if it's not in your $PATH. hm, do inline comments work with continuations? is this copy-pastable-? https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:92: (This should be fixed by http://reviews.llvm.org/D22046). "(This is fixed in llvm r274751, so if your llvm checkout is up-to-date this shouldn't be needed.)" instead Maybe just remove this section altogether though.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md File docs/clang_tidy.md (right): https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:18: provides a convenient interface for writing new checks. On 2016/07/12 18:44:09, Nico wrote: > i'd omit the last sentence Done. https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:53: ninja -C out/release chrome On 2016/07/12 18:44:09, Nico wrote: > upper-case R, then it works with gyp too It was my subtle hint to get people off gyp ;) But fair enough. Done. https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:57: ninja -C out/release -t compdb objxx cxx > compile_commands.json On 2016/07/12 18:44:09, Nico wrote: > ditto. (also, isn't it "objcxx" for mm files? not sure) Done, and yes! Typo. Thanks for catching it. https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:67: # Set the clang-tidy binary path, if it's not in your $PATH. On 2016/07/12 18:44:09, Nico wrote: > hm, do inline comments work with continuations? is this copy-pastable-? Nope, but I figured it was the easiest way of documenting what each part did. I've added a second "copy-paste friendly" version below it. (If you'd prefer, we could also have the command and then describe everything below, but I slightly prefer the inline approach.) https://codereview.chromium.org/2139883002/diff/20001/docs/clang_tidy.md#newc... docs/clang_tidy.md:92: (This should be fixed by http://reviews.llvm.org/D22046). On 2016/07/12 18:44:09, Nico wrote: > "(This is fixed in llvm r274751, so if your llvm checkout is up-to-date this > shouldn't be needed.)" instead > > Maybe just remove this section altogether though. Updated the phrasing. I'd like to keep it in for helping anyone who has a slightly outdated checkout. We can remove it in awhile when it's more likely everyone will have it.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2139883002/#ps60001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add clang-tidy documentation Add documentation for how to use clang-tidy in chromium. BUG=None ========== to ========== Add clang-tidy documentation Add documentation for how to use clang-tidy in chromium. BUG=None Committed: https://crrev.com/be2898eb2f8115fcee7a8cb9de17eff8cc6ee661 Cr-Commit-Position: refs/heads/master@{#404872} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/be2898eb2f8115fcee7a8cb9de17eff8cc6ee661 Cr-Commit-Position: refs/heads/master@{#404872} |
