Add url/ to banned_directories in clang style checker.

Add url/ to banned_directories in clang style checker.

BUG=229660
R=thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194481

[brettw, 2013-04-15 23:17:32]

We should add this back eventually when the problems are fixed.

[tfarina, 2013-04-16 00:06:21]

On Mon, Apr 15, 2013 at 8:17 PM, <brettw@chromium.org> wrote:

> We should add this back eventually when the problems are fixed.
>
> Yes. Actually I forgot to add a TODO to remove this when ready.


>
https://codereview.chromium.**org/14064011/<https://codereview.chromium.org/1...
>



-- 
Thiago

[tfarina, 2013-04-16 00:09:31]

Added the TODO.

Nico, please take another look.

Thanks!

[Nico, 2013-04-16 01:51:49]

lgtm either way. I was about to do a roll, but I suppose I'll wait for this to
land.

https://codereview.chromium.org/14064011/diff/5001/tools/clang/plugins/Chrome...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/14064011/diff/5001/tools/clang/plugins/Chrome...
tools/clang/plugins/ChromeClassTester.cpp:157:
banned_directories_.push_back("url/");
Do you want to put this behind a flag so that you can remove it without a clang
roll in the future?

[tfarina, 2013-04-16 03:05:50]

Nico, could you review it again and see if patch set 3 implements it correctly
behind a flag?

[Nico, 2013-04-16 15:31:11]

Looks basically right, but one question:

https://codereview.chromium.org/14064011/diff/8005/tools/clang/plugins/FindBa...
File tools/clang/plugins/FindBadConstructs.cpp (right):

https://codereview.chromium.org/14064011/diff/8005/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:83:
check_url_directory_(check_url_directory) {
Do you need this part? You're already passing this to the superclass
constructor. (Also, the member in the superclass is private – does this even
build?)

[tfarina, 2013-04-16 16:11:27]

On 2013/04/16 15:31:11, Nico wrote:
> Looks basically right, but one question:
> 
>
https://codereview.chromium.org/14064011/diff/8005/tools/clang/plugins/FindBa...
> File tools/clang/plugins/FindBadConstructs.cpp (right):
> 
>
https://codereview.chromium.org/14064011/diff/8005/tools/clang/plugins/FindBa...
> tools/clang/plugins/FindBadConstructs.cpp:83:
> check_url_directory_(check_url_directory) {
> Do you need this part? You're already passing this to the superclass
> constructor. (Also, the member in the superclass is private – does this even
> build?)

Looks like you are right. I was out of time. I just ran:

$ ./build/gyp_chromium -Dclang_use_chrome_plugins=1
$ ninja -C out/Debug/chrome

It seems I need to run ./tools/clang/scripts/update.sh --force_local_build, to
actually build the plugin.

I'll try this when I get back home, later tonight.

[Nico, 2013-04-16 17:21:44]

On Tue, Apr 16, 2013 at 9:11 AM, <tfarina@chromium.org> wrote:

> On 2013/04/16 15:31:11, Nico wrote:
>
>> Looks basically right, but one question:
>>
>
>
> https://codereview.chromium.**org/14064011/diff/8005/tools/**
>
clang/plugins/**FindBadConstructs.cpp<https://codereview.chromium.org/14064011/diff/8005/tools/clang/plugins/FindBadConstructs.cpp>
>
>> File tools/clang/plugins/**FindBadConstructs.cpp (right):
>>
>
>
> https://codereview.chromium.**org/14064011/diff/8005/tools/**
>
clang/plugins/**FindBadConstructs.cpp#**newcode83<https://codereview.chromium.org/14064011/diff/8005/tools/clang/plugins/FindBadConstructs.cpp#newcode83>
>
>> tools/clang/plugins/**FindBadConstructs.cpp:83:
>> check_url_directory_(check_**url_directory) {
>> Do you need this part? You're already passing this to the superclass
>> constructor. (Also, the member in the superclass is private – does this
>> even
>> build?)
>>
>
> Looks like you are right. I was out of time. I just ran:
>
> $ ./build/gyp_chromium -Dclang_use_chrome_plugins=1
>

(FWIW, the plugin is enabled by default when clang is enabled.)


> $ ninja -C out/Debug/chrome
>
> It seems I need to run ./tools/clang/scripts/update.**sh
> --force_local_build, to
> actually build the plugin.
>

Yes, that's correct.


> I'll try this when I get back home, later tonight.


>
https://codereview.chromium.**org/14064011/<https://codereview.chromium.org/1...
>

[tfarina, 2013-04-16 22:03:54]

Nico, could you review patch set 4 again?

You were right, there was two errors.

Now I built it with:

$ ./tools/clang/scripts/update.sh --force-local-build --without-android

[Nico, 2013-04-16 22:16:56]

https://codereview.chromium.org/14064011/diff/19001/tools/clang/plugins/Chrom...
File tools/clang/plugins/ChromeClassTester.h (right):

https://codereview.chromium.org/14064011/diff/19001/tools/clang/plugins/Chrom...
tools/clang/plugins/ChromeClassTester.h:49: bool check_url_directory_;
I think this can stay private. You only use this in ChromeClassTester.cpp.

[tfarina, 2013-04-16 22:23:24]

https://codereview.chromium.org/14064011/diff/19001/tools/clang/plugins/Chrom...
File tools/clang/plugins/ChromeClassTester.h (right):

https://codereview.chromium.org/14064011/diff/19001/tools/clang/plugins/Chrom...
tools/clang/plugins/ChromeClassTester.h:49: bool check_url_directory_;
On 2013/04/16 22:16:56, Nico wrote:
> I think this can stay private. You only use this in ChromeClassTester.cpp.

Done.

[Nico, 2013-04-16 22:35:56]

lgtm

[tfarina, 2013-04-16 23:22:34]

Committed patchset #5 manually as r194481 (presubmit successful).