This is an archive of the discontinued LLVM Phabricator instance.

[gn] Checking in BUILD.gn files for clang-tidy and clang-apply-replacements
ClosedPublic

Authored by mbonadei on Jan 28 2019, 7:48 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mbonadei created this revision.Jan 28 2019, 7:48 AM
yvesg accepted this revision.Feb 1 2019, 10:47 AM
yvesg added a subscriber: yvesg.

Nice, LGTM!

llvm/utils/gn/build/BUILD.gn
152 ↗(On Diff #183860)

I'm surprised there is a config for this, but not for clang-tidy itself. Why is that so?

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn
19 ↗(On Diff #183860)

What should be fixed? I.e. what is the current limitation?
Static Analyzer checks not available? If so, the following deps could be commented.

This revision is now accepted and ready to land.Feb 1 2019, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 10:47 AM

(Please give me a few more days too. Sorry it's taking so long, have been firefighting all week :-/)

(Please give me a few more days too. Sorry it's taking so long, have been firefighting all week :-/)

No worries. It is not urgent, I am also a bit swamped over here too. I will get back to this in a couple of days.

Thanks, this is great!

llvm/utils/gn/build/BUILD.gn
152 ↗(On Diff #183860)

I think we should remove this and instead put include_dirs = [ "include" ] and include_dirs = [ "../include" ] respectively in the two clang-apply-replacement build files instead. I think the clang-extra-tools stuff is intended to look like tools in llvm/tools or clang/tools and not like a full lib, and clang-apply-replacements is just weird in doing this full include and lib subtree thing for what is effectively 3 files.

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn
19 ↗(On Diff #183860)

+1.

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/abseil/BUILD.gn
14 ↗(On Diff #183860)

Because it took me so long, this is now missing

"DurationAdditionCheck.cpp",

(courtesy of llvm/utils/gn/build/sync_source_lists_from_cmake.py after rebasing locally)

llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
3 ↗(On Diff #183860)

Instead of this, I think it'd be better to say configs += [ "//llvm/utils/gn/build:clang_code" ] in all the clang-tools-extra targets (like all the clang targets do)

mbonadei updated this revision to Diff 185295.Feb 5 2019, 6:24 AM
mbonadei marked 7 inline comments as done.
mbonadei added inline comments.
llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn
19 ↗(On Diff #183860)

I commented out the two deps below. I will look into it in the next few days (ClangSACheckers uses tablegen and I need to read about it and learn what it does). Is it OK to land this without SACheckers support?

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/abseil/BUILD.gn
14 ↗(On Diff #183860)

That script is great! It detected two more checks that have been added recently! :)

llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
3 ↗(On Diff #183860)

I see that we are not declaring headers to gn. Is this something we'll want to have in the future? I think this is related to how we look at configs vs public_configs as well.

thakis accepted this revision.Feb 5 2019, 6:56 AM

lgtm!

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn
19 ↗(On Diff #183860)

Sure, that's fine. (git grep tablegen llvm/utils/gn will show you lots of tablegen examples if you want to tackle these later.)

llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
3 ↗(On Diff #183860)

Maybe; it's the gn check item in llvm/utils/gn/TODO.txt.

mbonadei marked an inline comment as done.Feb 5 2019, 7:10 AM
mbonadei added inline comments.
llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn
19 ↗(On Diff #183860)

Sure, I will do it! Thanks!

I don't have commit access. Can you take care of landing this one?

This revision was automatically updated to reflect the committed changes.