Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780113002:
Refactor banned directory checking so Blink can opt into certain checks. (Closed)
DescriptionRefactor banned directory checking so Blink can opt into certain checks.
BUG=706466
Review-Url: https://codereview.chromium.org/2780113002
Cr-Commit-Position: refs/heads/master@{#463748}
Committed: https://chromium.googlesource.com/chromium/src/+/6aff6f114be3dea567abd0409e07ea7a394c7527
Patch Set 1 #
Total comments: 12
Patch Set 2 : better enum names #
Messages
Total messages: 20 (7 generated)
Description was changed from ========== Refactor banned directory checking so Blink can opt into certain checks. BUG=none ========== to ========== Refactor banned directory checking so Blink can opt into certain checks. BUG=none ==========
dcheng@chromium.org changed reviewers: + thakis@chromium.org, tkent@chromium.org
https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... File tools/clang/plugins/ChromeClassTester.cpp (right): https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... tools/clang/plugins/ChromeClassTester.cpp:51: if (InBannedNamespace(tag)) Consolidating some common logic here. https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... tools/clang/plugins/ChromeClassTester.cpp:75: // TODO(dcheng): This should probably consult a separate list. We may event want separate ignore lists for Blink vs not-Blink but that's out of scope for now. https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/FindBad... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/FindBad... tools/clang/plugins/FindBadConstructsConsumer.cpp:466: bool ignored = type == LocationType::kIgnored || type == LocationType::kBlink; In the future, when location type is kBlink, we won't set ignore to true for certain diagnostic IDs. https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/FindBad... tools/clang/plugins/FindBadConstructsConsumer.cpp:625: if (type == LocationType::kIgnored || type == LocationType::kBlink) Ditto: this is needed for now, but this should be delegating to ReportIfSpellingLocNotIgnored(). I'll remove emitWarning() in a followup, eliminating this check. https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/FindBad... tools/clang/plugins/FindBadConstructsConsumer.cpp:1017: location_type != LocationType::kIgnored) { Note: We only ignore kIgnored here: kBlink gets filtered out by the ReportIfSpellingLocNotIgnored() checks.
Is there a bug with a rough plan outline somewhere? Why do we only want certain checks in blink, not all? I think there's already a plugin flag to make it not stop third_party/webkit (but I might be misremembering).
On 2017/03/29 12:29:12, Nico wrote: > Is there a bug with a rough plan outline somewhere? I haven't really written down a plan; my general thought is that we should be enabling more of the plugin checks in Blink. > Why do we only want certain > checks in blink, not all? I think there's already a plugin flag to make it not > stop third_party/webkit (but I might be misremembering). There is a flag, but it's all-or-nothing. There are some checks such as virtual inline bodies that we likely won't enable in the forseeable future for Blink, but many other checks (auto is qualified with * or &, override/final are used as appropriate) are valuable and should be enabled.
On 2017/03/29 16:42:19, dcheng wrote: > On 2017/03/29 12:29:12, Nico wrote: > > Is there a bug with a rough plan outline somewhere? > > I haven't really written down a plan; my general thought is that we should be > enabling more of the plugin checks in Blink. > > > Why do we only want certain > > checks in blink, not all? I think there's already a plugin flag to make it not > > stop third_party/webkit (but I might be misremembering). > > There is a flag, but it's all-or-nothing. There are some checks such as virtual > inline bodies that we likely won't enable in the forseeable future for Blink, > but many other checks (auto is qualified with * or &, override/final are used as > appropriate) are valuable and should be enabled. (I'll create a bug when I get into the office to track this)
Description was changed from ========== Refactor banned directory checking so Blink can opt into certain checks. BUG=none ========== to ========== Refactor banned directory checking so Blink can opt into certain checks. BUG=706466 ==========
Filed https://crbug.com/706466 for tracking this.
lgtm https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... File tools/clang/plugins/ChromeClassTester.cpp (left): https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... tools/clang/plugins/ChromeClassTester.cpp:241: if (options_.enforce_in_thirdparty_webkit) { We can remove Options::enforce_in_thirdparty_webkit.
https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... File tools/clang/plugins/ChromeClassTester.cpp (left): https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... tools/clang/plugins/ChromeClassTester.cpp:241: if (options_.enforce_in_thirdparty_webkit) { On 2017/04/06 04:29:56, tkent wrote: > We can remove Options::enforce_in_thirdparty_webkit. I was intentionally leaving this to provide a flag we can toggle while we incrementally enable checks. I guess we could try to make it more general instead (e.g. provide a flag that lets you specify a set of checks to enable), though I'm not sure how much plugin complexity this would add. I'll investigate this in a followup.
thakis, are you OK with proceeding with the plan listed in the bug?
lgtm, sounds good. This change here is behavior-preserving, right? And it shouldn't affect compile speed? https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... File tools/clang/plugins/ChromeClassTester.cpp (right): https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... tools/clang/plugins/ChromeClassTester.cpp:170: for (const std::string& banned_dir : banned_directories_) { shouldn't this be checked before blink_directories? https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... tools/clang/plugins/ChromeClassTester.cpp:186: return banned_namespaces_.find(n) != banned_namespaces_.end(); :-) https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... File tools/clang/plugins/ChromeClassTester.h (right): https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... tools/clang/plugins/ChromeClassTester.h:48: kIgnored, kNotIgnored, kIgnored, kBlink sounds a bit like TRUE, FALSE, FILE_NOT_FOUND – maybe kChromium, kBlink, kThirdParty (or kIgnored)? With comments explaining that kChromium means default checks, kBlink is a transitionary thing (hopefully, maybe, with link to your bug), and that kThirdParty (kIgnored) is for code where we don't diag.
https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... File tools/clang/plugins/ChromeClassTester.cpp (right): https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... tools/clang/plugins/ChromeClassTester.cpp:170: for (const std::string& banned_dir : banned_directories_) { On 2017/04/06 19:03:13, Nico wrote: > shouldn't this be checked before blink_directories? third_party is in the banned list, but we don't want to classify Blink as banned. So I think we want to still go in allowed list, banned list order of checking. https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... File tools/clang/plugins/ChromeClassTester.h (right): https://codereview.chromium.org/2780113002/diff/1/tools/clang/plugins/ChromeC... tools/clang/plugins/ChromeClassTester.h:48: kIgnored, On 2017/04/06 19:03:13, Nico wrote: > kNotIgnored, kIgnored, kBlink sounds a bit like TRUE, FALSE, FILE_NOT_FOUND – > maybe kChromium, kBlink, kThirdParty (or kIgnored)? With comments explaining > that kChromium means default checks, kBlink is a transitionary thing (hopefully, > maybe, with link to your bug), and that kThirdParty (kIgnored) is for code where > we don't diag. Done.
Had a brief discussion with thakis@ offline. In the long term, we'd like to move to some place where kThirdParty really means only third-party, even though it covers code generated by Chrome code generators today. Even though that's not the case today, we'll keep the kThirdParty label for now under the hopes that we can move this way in the long run.
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2780113002/#ps20001 (title: "better enum names")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491941386557800,
"parent_rev": "2b6b927cc9cf4442b160da02285ae62613f90d5f", "commit_rev":
"6aff6f114be3dea567abd0409e07ea7a394c7527"}
Message was sent while issue was closed.
Description was changed from ========== Refactor banned directory checking so Blink can opt into certain checks. BUG=706466 ========== to ========== Refactor banned directory checking so Blink can opt into certain checks. BUG=706466 Review-Url: https://codereview.chromium.org/2780113002 Cr-Commit-Position: refs/heads/master@{#463748} Committed: https://chromium.googlesource.com/chromium/src/+/6aff6f114be3dea567abd0409e07... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/6aff6f114be3dea567abd0409e07... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
