Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12746010:
Implement clang tool that converts std::string("") to std::string(). (Closed)
|
Created:
7 years, 9 months ago by dcheng Modified:
7 years, 8 months ago CC:
chromium-reviews, eugenis+clang_chromium.org, Dai Mikurube (NOT FULLTIME), glider+clang_chromium.org, glotov+watch_chromium.org, ukai+watch_chromium.org, djasper_google.com, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement clang tool that converts std::string("") to std::string().
This is intended to be a simple demo of the clang tools functionality.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191936
Patch Set 1 #Patch Set 2 : Now it works #Patch Set 3 : Cleanup #Patch Set 4 : Now with a half-working helper script. #Patch Set 5 : Rough draft #Patch Set 6 : Fix some bugs in run_tool.py #Patch Set 7 : Whee #
Total comments: 14
Patch Set 8 : Hopefully less silly. #Patch Set 9 : Hopefully less silly. #Patch Set 10 : ... #Patch Set 11 : Update tool and rewriter to handle initializers. #
Total comments: 4
Patch Set 12 : Fix comment typo #Patch Set 13 : Makefile and update.sh tweaks #Patch Set 14 : More script cleanup #
Total comments: 13
Patch Set 15 : Incorporate some feedback. #Patch Set 16 : A test harness. #Patch Set 17 : Remove debugging print and add comment. #
Total comments: 1
Patch Set 18 : Moar comments. #Patch Set 19 : Add copyright boilerplate to test files. Yay. #
Total comments: 1
Messages
Total messages: 32 (0 generated)
Hey Nico, What do you think of the basic approach in this CL? The tool is mostly intended as a demo of what is possible with a CLang tool. I did try running it across Chrome, and it only made an eighteen kilobyte difference =D
Thanks for working on this! https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... File tools/clang/empty_string/EmptyStringConverter.cpp (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:19: // TODO(dcheng): Figure out copyright blobs... Is there any reason this can't just have the regular chromium header? https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... File tools/clang/empty_string/Makefile (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... tools/clang/empty_string/Makefile:3: # The LLVM Compiler Infrastructure Same license question https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... tools/clang/empty_string/Makefile:15: # No plugins, optimize startup time. Does this have a measurable impact? https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/run_t... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/run_t... tools/clang/scripts/run_tool.py:4: # found in the LICENSE file. Doesn't say why this is used instead of tooling's write-to-disk stuff (like the comment in the previous file promised) https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/run_t... tools/clang/scripts/run_tool.py:26: return [f for f in output.splitlines() if f[-3:] == '.cc'] f.endswith('.cc') (there's also .m, .mm, .cpp, .h?) Are there cases where one doesn't want to run a tool on all files in the compilation db? https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/updat... File tools/clang/scripts/update.sh (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/updat... tools/clang/scripts/update.sh:378: CHROME_TOOL_DIRS="empty_string plugins" Hm, this means that everyone building clang locally will build lots of tools over time. Maybe there could be a build-tool $toolname script instead, which does the stuff below? Then people who want to use a tool can call that for the tool they care about, and update.sh could call it to build the plugin.
https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... File tools/clang/empty_string/EmptyStringConverter.cpp (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:19: // TODO(dcheng): Figure out copyright blobs... On 2013/03/27 16:35:29, Nico wrote: > Is there any reason this can't just have the regular chromium header? Done. https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... File tools/clang/empty_string/Makefile (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... tools/clang/empty_string/Makefile:3: # The LLVM Compiler Infrastructure On 2013/03/27 16:35:29, Nico wrote: > Same license question I've reimplemented the Makefile (I know, it looks basically the same) so now we can use the Chrome header. https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... tools/clang/empty_string/Makefile:15: # No plugins, optimize startup time. On 2013/03/27 16:35:29, Nico wrote: > Does this have a measurable impact? On a relatively small sample (~47 second runs)... nope. =/ But all the tools checked into Clang seem to have this stanza in their Makefile. This is supposedly a big optimization (4x difference in dyld startup time) for darwin. https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/run_t... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/run_t... tools/clang/scripts/run_tool.py:4: # found in the LICENSE file. On 2013/03/27 16:35:29, Nico wrote: > Doesn't say why this is used instead of tooling's write-to-disk stuff (like the > comment in the previous file promised) Done now =) https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/run_t... tools/clang/scripts/run_tool.py:26: return [f for f in output.splitlines() if f[-3:] == '.cc'] On 2013/03/27 16:35:29, Nico wrote: > f.endswith('.cc') > > (there's also .m, .mm, .cpp, .h?) > > > Are there cases where one doesn't want to run a tool on all files in the > compilation db? 1) I didn't really want to parse compile_commands.json. The coupling between the Clang tool and this python script scraping the output is yucky, and parsing compile_commands.json would add another coupling. 2) It doesn't need to run across generated files, since there's no point to processing those anyway. Without any filtering of the output from git ls-files, the script is only 12% complete at the 7 minute mark (implying a ~56m run time assuming a constant rate of progress). With the current .cc filtering, the script takes 30 minutes. I've updated the filter to be more inclusive though and added some of the suggested extensions since we definitely do want to rewrite those files as well. I've left out .h (since these will be rewritten anyway if they're included anywhere that's compiled) and .cpp (since we shouldn't have those in the Chrome tree). https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/updat... File tools/clang/scripts/update.sh (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/scripts/updat... tools/clang/scripts/update.sh:378: CHROME_TOOL_DIRS="empty_string plugins" On 2013/03/27 16:35:29, Nico wrote: > Hm, this means that everyone building clang locally will build lots of tools > over time. Maybe there could be a build-tool $toolname script instead, which > does the stuff below? Then people who want to use a tool can call that for the > tool they care about, and update.sh could call it to build the plugin. I added (another) command-line argument instead, so I can just use $LLVM_DIR and ${NUM_JOBS}. It defaults to just plugins so people who don't care about tooling can just omit the argument completely.
This is looking great. Should we ask klimek@ for feedback on this? (He's the google-internal matcher expert.) If not, I'll take a look at the matcher stuff (likely just minor stuff) myself. https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... File tools/clang/empty_string/Makefile (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... tools/clang/empty_string/Makefile:15: # No plugins, optimize startup time. On 2013/03/28 00:10:17, dcheng wrote: > On 2013/03/27 16:35:29, Nico wrote: > > Does this have a measurable impact? > > On a relatively small sample (~47 second runs)... nope. =/ > But all the tools checked into Clang seem to have this stanza in their Makefile. > This is supposedly a big optimization (4x difference in dyld startup time) for > darwin. We don't build regular clang with it (else the plugin wouldn't work). If you can't measure the impact of this, I'd drop this. https://codereview.chromium.org/12746010/diff/32001/tools/clang/scripts/run_t... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/12746010/diff/32001/tools/clang/scripts/run_t... tools/clang/scripts/run_tool.py:23: across Chromium, regardless of whether some instances failed or not. Ok, I can see this being useful. Maybe we should talk to klimek@ about moving these features upstream though eventually. https://codereview.chromium.org/12746010/diff/32001/tools/clang/scripts/updat... File tools/clang/scripts/update.sh (right): https://codereview.chromium.org/12746010/diff/32001/tools/clang/scripts/updat... tools/clang/scripts/update.sh:93: "Defaults to plugins." Maybe list `--with-chrome-tools='plugins empty-string'` as example to make it clear that this can a) list multiple plugins b) they are space separated.
This is actually modeled on some other tools I've worked on/written, so I think we probably don't need to loop klimek@ into this one. So if you don't mind taking a look yourself, that'd be great. https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... File tools/clang/empty_string/Makefile (right): https://codereview.chromium.org/12746010/diff/12001/tools/clang/empty_string/... tools/clang/empty_string/Makefile:15: # No plugins, optimize startup time. On 2013/03/29 22:30:36, Nico wrote: > On 2013/03/28 00:10:17, dcheng wrote: > > On 2013/03/27 16:35:29, Nico wrote: > > > Does this have a measurable impact? > > > > On a relatively small sample (~47 second runs)... nope. =/ > > But all the tools checked into Clang seem to have this stanza in their > Makefile. > > This is supposedly a big optimization (4x difference in dyld startup time) for > > darwin. > > We don't build regular clang with it (else the plugin wouldn't work). If you > can't measure the impact of this, I'd drop this. I'll take it out for now and measure on OS X later. https://codereview.chromium.org/12746010/diff/32001/tools/clang/scripts/run_t... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/12746010/diff/32001/tools/clang/scripts/run_t... tools/clang/scripts/run_tool.py:23: across Chromium, regardless of whether some instances failed or not. On 2013/03/29 22:30:36, Nico wrote: > Ok, I can see this being useful. Maybe we should talk to klimek@ about moving > these features upstream though eventually. I've had this discussion with them internally already. I think it will be upstreamed eventually but it's not going to happen just yet. https://codereview.chromium.org/12746010/diff/32001/tools/clang/scripts/updat... File tools/clang/scripts/update.sh (right): https://codereview.chromium.org/12746010/diff/32001/tools/clang/scripts/updat... tools/clang/scripts/update.sh:93: "Defaults to plugins." On 2013/03/29 22:30:36, Nico wrote: > Maybe list `--with-chrome-tools='plugins empty-string'` as example to make it > clear that this can a) list multiple plugins b) they are space separated. Done.
I've added Klimek@ as a reviewer.
This looks good as far as I can tell, but I'm not a matcher expert. Last comment: Can you think of a simple way to test this? Worst case, some form of shell script that just runs this for a few known inputs and checks the outputs? https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... File tools/clang/empty_string/EmptyStringConverter.cpp (right): https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:4: Add a comment somewhere that gives a short overview of what this does https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:41: kTemporaryMode, nit: trailing commas in enums are a c++11 feature https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:46: EmptyStringConverterCallback(ReplacementMode mode, Replacements* replacements) Maybe you want to use llvm style instead of chromium style for this? (in that case, "type *varname", or pass -style LLVM to clang-format) https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:49: virtual void run(const MatchFinder::MatchResult& result); There's LLVM_OVERRIDE I think (which expands to nothing in c++98 mode, but it does serve as documentation at least for now) https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:72: const auto& constructor_call = …oh, you're going with c++11 mode. nevermind the comma comment above (and LLVM_OVERRIDE will even have a semantic meaning then) https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:72: const auto& constructor_call = Also, since this is meant to be a demo, maybe add a short comment that explains how to read matchers (or, better yet, include a link to the matcher docs)
One of the things that happen when we're not fast enough on the upstream track :( I'd like to see things along those lines in upstream clang instead of having shadow versions in chrome. We have plans to upstream some of our infrastructure around large scale changes where we recently started to develop better abstractions to be able to plug in serialization etc, and you'll get clang-format integration and correct deduplication and application of changes for free. https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... File tools/clang/empty_string/EmptyStringConverter.cpp (right): https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:103: switch (mode_) { I'd use 3 classes instead of the switch.
On 2013/04/01 16:30:33, klimek wrote:
> One of the things that happen when we're not fast enough on the upstream track
> :( I'd like to see things along those lines
Do you mean a string("") -> string() tool? Or do you mean the python bits?
> in upstream clang instead of having
> shadow versions in chrome. We have plans to upstream some of our
infrastructure
> around large scale changes where we recently started to develop better
> abstractions to be able to plug in serialization etc, and you'll get
> clang-format integration and correct deduplication and application of changes
> for free.
>
>
https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/...
> File tools/clang/empty_string/EmptyStringConverter.cpp (right):
>
>
https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/...
> tools/clang/empty_string/EmptyStringConverter.cpp:103: switch (mode_) {
> I'd use 3 classes instead of the switch.
On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: > On 2013/04/01 16:30:33, klimek wrote: > >> One of the things that happen when we're not fast enough on the upstream >> track >> :( I'd like to see things along those lines >> > > Do you mean a string("") -> string() tool? Or do you mean the python bits? I mean: 1. good examples on how to write tests upstream (without FileTest) 2. an abstraction for handling lists of replacements, where serialization and deserialization is plug-inable, and which is "applicable" on a file 3. a tool that slurps in a set of those lists of replacements and applies them (note that you'll want more than replacements, for example when you want to add a header) Cheers, /Manuel > > > in upstream clang instead of having >> shadow versions in chrome. We have plans to upstream some of our >> > infrastructure > >> around large scale changes where we recently started to develop better >> abstractions to be able to plug in serialization etc, and you'll get >> clang-format integration and correct deduplication and application of >> changes >> for free. >> > > > https://codereview.chromium.**org/12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> > >> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >> > > > https://codereview.chromium.**org/12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> > >> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch (mode_) { >> I'd use 3 classes instead of the switch. >> > > > > https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >
On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com> wrote: > On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: > >> On 2013/04/01 16:30:33, klimek wrote: >> >>> One of the things that happen when we're not fast enough on the upstream >>> track >>> :( I'd like to see things along those lines >>> >> >> Do you mean a string("") -> string() tool? Or do you mean the python bits? > > > I mean: > 1. good examples on how to write tests upstream (without FileTest) > 2. an abstraction for handling lists of replacements, where serialization > and deserialization is plug-inable, and which is "applicable" on a file > 3. a tool that slurps in a set of those lists of replacements and applies > them (note that you'll want more than replacements, for example when you > want to add a header) > Sounds like "the python bits". I don't think it's terrible that we have this script until the upstream bits get ready. That way, Daniel (Cheng) use the tooling bits today, and you have as much time as you need to do the Right Thing upstream. > > Cheers, > /Manuel > > >> >> >> in upstream clang instead of having >>> shadow versions in chrome. We have plans to upstream some of our >>> >> infrastructure >> >>> around large scale changes where we recently started to develop better >>> abstractions to be able to plug in serialization etc, and you'll get >>> clang-format integration and correct deduplication and application of >>> changes >>> for free. >>> >> >> >> https://codereview.chromium.**org/12746010/diff/48001/tools/** >> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >> >>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >>> >> >> >> https://codereview.chromium.**org/12746010/diff/48001/tools/** >> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >> >>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch (mode_) >>> { >>> I'd use 3 classes instead of the switch. >>> >> >> >> >> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >> > >
On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org> wrote: > On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com> wrote: > >> On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: >> >>> On 2013/04/01 16:30:33, klimek wrote: >>> >>>> One of the things that happen when we're not fast enough on the >>>> upstream track >>>> :( I'd like to see things along those lines >>>> >>> >>> Do you mean a string("") -> string() tool? Or do you mean the python >>> bits? >> >> >> I mean: >> 1. good examples on how to write tests upstream (without FileTest) >> 2. an abstraction for handling lists of replacements, where serialization >> and deserialization is plug-inable, and which is "applicable" on a file >> 3. a tool that slurps in a set of those lists of replacements and applies >> them (note that you'll want more than replacements, for example when you >> want to add a header) >> > > Sounds like "the python bits". > > I don't think it's terrible that we have this script until the upstream > bits get ready. That way, Daniel (Cheng) use the tooling bits today, and > you have as much time as you need to do the Right Thing upstream. > I agree. I mainly mourn the opportunity cost due to us being too slow, and I wanted you guys to be aware of the plans so the trade-offs involved when you find bugs in those parts are clear :) > > >> >> Cheers, >> /Manuel >> >> >>> >>> >>> in upstream clang instead of having >>>> shadow versions in chrome. We have plans to upstream some of our >>>> >>> infrastructure >>> >>>> around large scale changes where we recently started to develop better >>>> abstractions to be able to plug in serialization etc, and you'll get >>>> clang-format integration and correct deduplication and application of >>>> changes >>>> for free. >>>> >>> >>> >>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >>> >>>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >>>> >>> >>> >>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >>> >>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch >>>> (mode_) { >>>> I'd use 3 classes instead of the switch. >>>> >>> >>> >>> >>> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >>> >> >> >
So, I might have gotten the earlier mail thread with Daniel wrong, but what is the purpose of tools like this? From the earlier discussion I gathered that the problem would be much better addressed with ClangMRs running over the Chromium codebase. What's missing seem to be working indexes of Windows and Mac compiles. If such a tool runs on an entire compilation database as Nico is suggesting in one of the comments, this would take quite long (days?). While, I agree that we can upstream some of the stuff we have, I think it is quite a bit to go until we have tools that can reasonably work on a code base as large as Chrome's. On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org> wrote: > On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com> wrote: > >> On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: >> >>> On 2013/04/01 16:30:33, klimek wrote: >>> >>>> One of the things that happen when we're not fast enough on the >>>> upstream track >>>> :( I'd like to see things along those lines >>>> >>> >>> Do you mean a string("") -> string() tool? Or do you mean the python >>> bits? >> >> >> I mean: >> 1. good examples on how to write tests upstream (without FileTest) >> 2. an abstraction for handling lists of replacements, where serialization >> and deserialization is plug-inable, and which is "applicable" on a file >> 3. a tool that slurps in a set of those lists of replacements and applies >> them (note that you'll want more than replacements, for example when you >> want to add a header) >> > > Sounds like "the python bits". > > I don't think it's terrible that we have this script until the upstream > bits get ready. That way, Daniel (Cheng) use the tooling bits today, and > you have as much time as you need to do the Right Thing upstream. > > >> >> Cheers, >> /Manuel >> >> >>> >>> >>> in upstream clang instead of having >>>> shadow versions in chrome. We have plans to upstream some of our >>>> >>> infrastructure >>> >>>> around large scale changes where we recently started to develop better >>>> abstractions to be able to plug in serialization etc, and you'll get >>>> clang-format integration and correct deduplication and application of >>>> changes >>>> for free. >>>> >>> >>> >>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >>> >>>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >>>> >>> >>> >>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >>> >>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch >>>> (mode_) { >>>> I'd use 3 classes instead of the switch. >>>> >>> >>> >>> >>> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >>> >> >> >
On Mon, Apr 1, 2013 at 9:46 AM, Daniel Jasper <djasper@google.com> wrote: > So, I might have gotten the earlier mail thread with Daniel wrong, but > what is the purpose of tools like this? From the earlier discussion I > gathered that the problem would be much better addressed with ClangMRs > running over the Chromium codebase. What's missing seem to be working > indexes of Windows and Mac compiles. > > If such a tool runs on an entire compilation database as Nico is > suggesting in one of the comments, this would take quite long (days?) > No, this should be at least as fast as building chrome, so it'd take less than an hour I think. dcheng can probably give you a concrete number. > . While, I agree that we can upstream some of the stuff we have, I think > it is quite a bit to go until we have tools that can reasonably work on a > code base as large as Chrome's. > > > On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org> wrote: > >> On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com> wrote: >> >>> On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: >>> >>>> On 2013/04/01 16:30:33, klimek wrote: >>>> >>>>> One of the things that happen when we're not fast enough on the >>>>> upstream track >>>>> :( I'd like to see things along those lines >>>>> >>>> >>>> Do you mean a string("") -> string() tool? Or do you mean the python >>>> bits? >>> >>> >>> I mean: >>> 1. good examples on how to write tests upstream (without FileTest) >>> 2. an abstraction for handling lists of replacements, where >>> serialization and deserialization is plug-inable, and which is "applicable" >>> on a file >>> 3. a tool that slurps in a set of those lists of replacements and >>> applies them (note that you'll want more than replacements, for example >>> when you want to add a header) >>> >> >> Sounds like "the python bits". >> >> I don't think it's terrible that we have this script until the upstream >> bits get ready. That way, Daniel (Cheng) use the tooling bits today, and >> you have as much time as you need to do the Right Thing upstream. >> >> >>> >>> Cheers, >>> /Manuel >>> >>> >>>> >>>> >>>> in upstream clang instead of having >>>>> shadow versions in chrome. We have plans to upstream some of our >>>>> >>>> infrastructure >>>> >>>>> around large scale changes where we recently started to develop better >>>>> abstractions to be able to plug in serialization etc, and you'll get >>>>> clang-format integration and correct deduplication and application of >>>>> changes >>>>> for free. >>>>> >>>> >>>> >>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >>>> >>>>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >>>>> >>>> >>>> >>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >>>> >>>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch >>>>> (mode_) { >>>>> I'd use 3 classes instead of the switch. >>>>> >>>> >>>> >>>> >>>> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >>>> >>> >>> >> >
On Mon, Apr 1, 2013 at 6:50 PM, Nico Weber <thakis@chromium.org> wrote: > On Mon, Apr 1, 2013 at 9:46 AM, Daniel Jasper <djasper@google.com> wrote: > >> So, I might have gotten the earlier mail thread with Daniel wrong, but >> what is the purpose of tools like this? From the earlier discussion I >> gathered that the problem would be much better addressed with ClangMRs >> running over the Chromium codebase. What's missing seem to be working >> indexes of Windows and Mac compiles. >> >> If such a tool runs on an entire compilation database as Nico is >> suggesting in one of the comments, this would take quite long (days?) >> > > No, this should be at least as fast as building chrome, so it'd take less > than an hour I think. dcheng can probably give you a concrete number. > +1, I'd expect this to not take too long on one of the chrome dev machines ;) > > >> . While, I agree that we can upstream some of the stuff we have, I think >> it is quite a bit to go until we have tools that can reasonably work on a >> code base as large as Chrome's. >> >> >> On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org> wrote: >> >>> On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com> wrote: >>> >>>> On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: >>>> >>>>> On 2013/04/01 16:30:33, klimek wrote: >>>>> >>>>>> One of the things that happen when we're not fast enough on the >>>>>> upstream track >>>>>> :( I'd like to see things along those lines >>>>>> >>>>> >>>>> Do you mean a string("") -> string() tool? Or do you mean the python >>>>> bits? >>>> >>>> >>>> I mean: >>>> 1. good examples on how to write tests upstream (without FileTest) >>>> 2. an abstraction for handling lists of replacements, where >>>> serialization and deserialization is plug-inable, and which is "applicable" >>>> on a file >>>> 3. a tool that slurps in a set of those lists of replacements and >>>> applies them (note that you'll want more than replacements, for example >>>> when you want to add a header) >>>> >>> >>> Sounds like "the python bits". >>> >>> I don't think it's terrible that we have this script until the upstream >>> bits get ready. That way, Daniel (Cheng) use the tooling bits today, and >>> you have as much time as you need to do the Right Thing upstream. >>> >>> >>>> >>>> Cheers, >>>> /Manuel >>>> >>>> >>>>> >>>>> >>>>> in upstream clang instead of having >>>>>> shadow versions in chrome. We have plans to upstream some of our >>>>>> >>>>> infrastructure >>>>> >>>>>> around large scale changes where we recently started to develop better >>>>>> abstractions to be able to plug in serialization etc, and you'll get >>>>>> clang-format integration and correct deduplication and application of >>>>>> changes >>>>>> for free. >>>>>> >>>>> >>>>> >>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >>>>> >>>>>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >>>>>> >>>>> >>>>> >>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >>>>> >>>>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch >>>>>> (mode_) { >>>>>> I'd use 3 classes instead of the switch. >>>>>> >>>>> >>>>> >>>>> >>>>> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >>>>> >>>> >>>> >>> >> >
Well, I had not taken a look at the recent changelist. However, it seems to me that most of what run_tool.py contains is the key part of the infrastructure that should be upstreamed. And that is also what I meant by "quite a bit to go". There is a significant amount of design and implementation that needs to be done before such tools become sane. I still don't fully understand the purpose of this tool. If it is meant as an example, we probably should get the infrastructure right, first. If you need this refactoring for Chrome now, why not use ClangMR? Does this tool work on Windows? On Mon, Apr 1, 2013 at 6:53 PM, Manuel Klimek <klimek@google.com> wrote: > On Mon, Apr 1, 2013 at 6:50 PM, Nico Weber <thakis@chromium.org> wrote: > >> On Mon, Apr 1, 2013 at 9:46 AM, Daniel Jasper <djasper@google.com> wrote: >> >>> So, I might have gotten the earlier mail thread with Daniel wrong, but >>> what is the purpose of tools like this? From the earlier discussion I >>> gathered that the problem would be much better addressed with ClangMRs >>> running over the Chromium codebase. What's missing seem to be working >>> indexes of Windows and Mac compiles. >>> >>> If such a tool runs on an entire compilation database as Nico is >>> suggesting in one of the comments, this would take quite long (days?) >>> >> >> No, this should be at least as fast as building chrome, so it'd take less >> than an hour I think. dcheng can probably give you a concrete number. >> > > +1, I'd expect this to not take too long on one of the chrome dev machines > ;) > > >> >> >>> . While, I agree that we can upstream some of the stuff we have, I think >>> it is quite a bit to go until we have tools that can reasonably work on a >>> code base as large as Chrome's. >>> >>> >>> On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org> wrote: >>> >>>> On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com>wrote: >>>> >>>>> On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: >>>>> >>>>>> On 2013/04/01 16:30:33, klimek wrote: >>>>>> >>>>>>> One of the things that happen when we're not fast enough on the >>>>>>> upstream track >>>>>>> :( I'd like to see things along those lines >>>>>>> >>>>>> >>>>>> Do you mean a string("") -> string() tool? Or do you mean the python >>>>>> bits? >>>>> >>>>> >>>>> I mean: >>>>> 1. good examples on how to write tests upstream (without FileTest) >>>>> 2. an abstraction for handling lists of replacements, where >>>>> serialization and deserialization is plug-inable, and which is "applicable" >>>>> on a file >>>>> 3. a tool that slurps in a set of those lists of replacements and >>>>> applies them (note that you'll want more than replacements, for example >>>>> when you want to add a header) >>>>> >>>> >>>> Sounds like "the python bits". >>>> >>>> I don't think it's terrible that we have this script until the upstream >>>> bits get ready. That way, Daniel (Cheng) use the tooling bits today, and >>>> you have as much time as you need to do the Right Thing upstream. >>>> >>>> >>>>> >>>>> Cheers, >>>>> /Manuel >>>>> >>>>> >>>>>> >>>>>> >>>>>> in upstream clang instead of having >>>>>>> shadow versions in chrome. We have plans to upstream some of our >>>>>>> >>>>>> infrastructure >>>>>> >>>>>>> around large scale changes where we recently started to develop >>>>>>> better >>>>>>> abstractions to be able to plug in serialization etc, and you'll get >>>>>>> clang-format integration and correct deduplication and application >>>>>>> of changes >>>>>>> for free. >>>>>>> >>>>>> >>>>>> >>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >>>>>> >>>>>>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >>>>>>> >>>>>> >>>>>> >>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >>>>>> >>>>>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch >>>>>>> (mode_) { >>>>>>> I'd use 3 classes instead of the switch. >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >>>>>> >>>>> >>>>> >>>> >>> >> >
On Mon, Apr 1, 2013 at 10:09 AM, Daniel Jasper <djasper@google.com> wrote: > Well, I had not taken a look at the recent changelist. However, it seems > to me that most of what run_tool.py contains is the key part of the > infrastructure that should be upstreamed. And that is also what I meant by > "quite a bit to go". There is a significant amount of design and > implementation that needs to be done before such tools become sane. > > I still don't fully understand the purpose of this tool. If it is meant as > an example, we probably should get the infrastructure right, first. If you > need this refactoring for Chrome now, why not use ClangMR? > ClangMR is for googlers only, right? > Does this tool work on Windows? > > > > On Mon, Apr 1, 2013 at 6:53 PM, Manuel Klimek <klimek@google.com> wrote: > >> On Mon, Apr 1, 2013 at 6:50 PM, Nico Weber <thakis@chromium.org> wrote: >> >>> On Mon, Apr 1, 2013 at 9:46 AM, Daniel Jasper <djasper@google.com>wrote: >>> >>>> So, I might have gotten the earlier mail thread with Daniel wrong, but >>>> what is the purpose of tools like this? From the earlier discussion I >>>> gathered that the problem would be much better addressed with ClangMRs >>>> running over the Chromium codebase. What's missing seem to be working >>>> indexes of Windows and Mac compiles. >>>> >>>> If such a tool runs on an entire compilation database as Nico is >>>> suggesting in one of the comments, this would take quite long (days?) >>>> >>> >>> No, this should be at least as fast as building chrome, so it'd take >>> less than an hour I think. dcheng can probably give you a concrete number. >>> >> >> +1, I'd expect this to not take too long on one of the chrome dev >> machines ;) >> >> >>> >>> >>>> . While, I agree that we can upstream some of the stuff we have, I >>>> think it is quite a bit to go until we have tools that can reasonably work >>>> on a code base as large as Chrome's. >>>> >>>> >>>> On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org> wrote: >>>> >>>>> On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com>wrote: >>>>> >>>>>> On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: >>>>>> >>>>>>> On 2013/04/01 16:30:33, klimek wrote: >>>>>>> >>>>>>>> One of the things that happen when we're not fast enough on the >>>>>>>> upstream track >>>>>>>> :( I'd like to see things along those lines >>>>>>>> >>>>>>> >>>>>>> Do you mean a string("") -> string() tool? Or do you mean the python >>>>>>> bits? >>>>>> >>>>>> >>>>>> I mean: >>>>>> 1. good examples on how to write tests upstream (without FileTest) >>>>>> 2. an abstraction for handling lists of replacements, where >>>>>> serialization and deserialization is plug-inable, and which is "applicable" >>>>>> on a file >>>>>> 3. a tool that slurps in a set of those lists of replacements and >>>>>> applies them (note that you'll want more than replacements, for example >>>>>> when you want to add a header) >>>>>> >>>>> >>>>> Sounds like "the python bits". >>>>> >>>>> I don't think it's terrible that we have this script until the >>>>> upstream bits get ready. That way, Daniel (Cheng) use the tooling bits >>>>> today, and you have as much time as you need to do the Right Thing upstream. >>>>> >>>>> >>>>>> >>>>>> Cheers, >>>>>> /Manuel >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>> in upstream clang instead of having >>>>>>>> shadow versions in chrome. We have plans to upstream some of our >>>>>>>> >>>>>>> infrastructure >>>>>>> >>>>>>>> around large scale changes where we recently started to develop >>>>>>>> better >>>>>>>> abstractions to be able to plug in serialization etc, and you'll get >>>>>>>> clang-format integration and correct deduplication and application >>>>>>>> of changes >>>>>>>> for free. >>>>>>>> >>>>>>> >>>>>>> >>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>>> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >>>>>>> >>>>>>>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >>>>>>>> >>>>>>> >>>>>>> >>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>>> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >>>>>>> >>>>>>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch >>>>>>>> (mode_) { >>>>>>>> I'd use 3 classes instead of the switch. >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
On Mon, Apr 1, 2013 at 7:25 PM, Nico Weber <thakis@chromium.org> wrote: > On Mon, Apr 1, 2013 at 10:09 AM, Daniel Jasper <djasper@google.com> wrote: > >> Well, I had not taken a look at the recent changelist. However, it seems >> to me that most of what run_tool.py contains is the key part of the >> infrastructure that should be upstreamed. And that is also what I meant by >> "quite a bit to go". There is a significant amount of design and >> implementation that needs to be done before such tools become sane. >> >> I still don't fully understand the purpose of this tool. If it is meant >> as an example, we probably should get the infrastructure right, first. If >> you need this refactoring for Chrome now, why not use ClangMR? >> > > ClangMR is for googlers only, right? > Yep. > > >> Does this tool work on Windows? >> >> >> >> On Mon, Apr 1, 2013 at 6:53 PM, Manuel Klimek <klimek@google.com> wrote: >> >>> On Mon, Apr 1, 2013 at 6:50 PM, Nico Weber <thakis@chromium.org> wrote: >>> >>>> On Mon, Apr 1, 2013 at 9:46 AM, Daniel Jasper <djasper@google.com>wrote: >>>> >>>>> So, I might have gotten the earlier mail thread with Daniel wrong, but >>>>> what is the purpose of tools like this? From the earlier discussion I >>>>> gathered that the problem would be much better addressed with ClangMRs >>>>> running over the Chromium codebase. What's missing seem to be working >>>>> indexes of Windows and Mac compiles. >>>>> >>>>> If such a tool runs on an entire compilation database as Nico is >>>>> suggesting in one of the comments, this would take quite long (days?) >>>>> >>>> >>>> No, this should be at least as fast as building chrome, so it'd take >>>> less than an hour I think. dcheng can probably give you a concrete number. >>>> >>> >>> +1, I'd expect this to not take too long on one of the chrome dev >>> machines ;) >>> >>> >>>> >>>> >>>>> . While, I agree that we can upstream some of the stuff we have, I >>>>> think it is quite a bit to go until we have tools that can reasonably work >>>>> on a code base as large as Chrome's. >>>>> >>>>> >>>>> On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org>wrote: >>>>> >>>>>> On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com>wrote: >>>>>> >>>>>>> On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: >>>>>>> >>>>>>>> On 2013/04/01 16:30:33, klimek wrote: >>>>>>>> >>>>>>>>> One of the things that happen when we're not fast enough on the >>>>>>>>> upstream track >>>>>>>>> :( I'd like to see things along those lines >>>>>>>>> >>>>>>>> >>>>>>>> Do you mean a string("") -> string() tool? Or do you mean the >>>>>>>> python bits? >>>>>>> >>>>>>> >>>>>>> I mean: >>>>>>> 1. good examples on how to write tests upstream (without FileTest) >>>>>>> 2. an abstraction for handling lists of replacements, where >>>>>>> serialization and deserialization is plug-inable, and which is "applicable" >>>>>>> on a file >>>>>>> 3. a tool that slurps in a set of those lists of replacements and >>>>>>> applies them (note that you'll want more than replacements, for example >>>>>>> when you want to add a header) >>>>>>> >>>>>> >>>>>> Sounds like "the python bits". >>>>>> >>>>>> I don't think it's terrible that we have this script until the >>>>>> upstream bits get ready. That way, Daniel (Cheng) use the tooling bits >>>>>> today, and you have as much time as you need to do the Right Thing upstream. >>>>>> >>>>>> >>>>>>> >>>>>>> Cheers, >>>>>>> /Manuel >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> in upstream clang instead of having >>>>>>>>> shadow versions in chrome. We have plans to upstream some of our >>>>>>>>> >>>>>>>> infrastructure >>>>>>>> >>>>>>>>> around large scale changes where we recently started to develop >>>>>>>>> better >>>>>>>>> abstractions to be able to plug in serialization etc, and you'll >>>>>>>>> get >>>>>>>>> clang-format integration and correct deduplication and application >>>>>>>>> of changes >>>>>>>>> for free. >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>>>> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >>>>>>>> >>>>>>>>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>>>> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >>>>>>>> >>>>>>>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch >>>>>>>>> (mode_) { >>>>>>>>> I'd use 3 classes instead of the switch. >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
Last I checked, dcheng is a Googler :-). On Mon, Apr 1, 2013 at 7:26 PM, Manuel Klimek <klimek@google.com> wrote: > On Mon, Apr 1, 2013 at 7:25 PM, Nico Weber <thakis@chromium.org> wrote: > >> On Mon, Apr 1, 2013 at 10:09 AM, Daniel Jasper <djasper@google.com>wrote: >> >>> Well, I had not taken a look at the recent changelist. However, it seems >>> to me that most of what run_tool.py contains is the key part of the >>> infrastructure that should be upstreamed. And that is also what I meant by >>> "quite a bit to go". There is a significant amount of design and >>> implementation that needs to be done before such tools become sane. >>> >>> I still don't fully understand the purpose of this tool. If it is meant >>> as an example, we probably should get the infrastructure right, first. If >>> you need this refactoring for Chrome now, why not use ClangMR? >>> >> >> ClangMR is for googlers only, right? >> > > Yep. > > >> >> >>> Does this tool work on Windows? >>> >>> >>> >>> On Mon, Apr 1, 2013 at 6:53 PM, Manuel Klimek <klimek@google.com> wrote: >>> >>>> On Mon, Apr 1, 2013 at 6:50 PM, Nico Weber <thakis@chromium.org> wrote: >>>> >>>>> On Mon, Apr 1, 2013 at 9:46 AM, Daniel Jasper <djasper@google.com>wrote: >>>>> >>>>>> So, I might have gotten the earlier mail thread with Daniel wrong, >>>>>> but what is the purpose of tools like this? From the earlier discussion I >>>>>> gathered that the problem would be much better addressed with ClangMRs >>>>>> running over the Chromium codebase. What's missing seem to be working >>>>>> indexes of Windows and Mac compiles. >>>>>> >>>>>> If such a tool runs on an entire compilation database as Nico is >>>>>> suggesting in one of the comments, this would take quite long (days?) >>>>>> >>>>> >>>>> No, this should be at least as fast as building chrome, so it'd take >>>>> less than an hour I think. dcheng can probably give you a concrete number. >>>>> >>>> >>>> +1, I'd expect this to not take too long on one of the chrome dev >>>> machines ;) >>>> >>>> >>>>> >>>>> >>>>>> . While, I agree that we can upstream some of the stuff we have, I >>>>>> think it is quite a bit to go until we have tools that can reasonably work >>>>>> on a code base as large as Chrome's. >>>>>> >>>>>> >>>>>> On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org>wrote: >>>>>> >>>>>>> On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com>wrote: >>>>>>> >>>>>>>> On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: >>>>>>>> >>>>>>>>> On 2013/04/01 16:30:33, klimek wrote: >>>>>>>>> >>>>>>>>>> One of the things that happen when we're not fast enough on the >>>>>>>>>> upstream track >>>>>>>>>> :( I'd like to see things along those lines >>>>>>>>>> >>>>>>>>> >>>>>>>>> Do you mean a string("") -> string() tool? Or do you mean the >>>>>>>>> python bits? >>>>>>>> >>>>>>>> >>>>>>>> I mean: >>>>>>>> 1. good examples on how to write tests upstream (without FileTest) >>>>>>>> 2. an abstraction for handling lists of replacements, where >>>>>>>> serialization and deserialization is plug-inable, and which is "applicable" >>>>>>>> on a file >>>>>>>> 3. a tool that slurps in a set of those lists of replacements and >>>>>>>> applies them (note that you'll want more than replacements, for example >>>>>>>> when you want to add a header) >>>>>>>> >>>>>>> >>>>>>> Sounds like "the python bits". >>>>>>> >>>>>>> I don't think it's terrible that we have this script until the >>>>>>> upstream bits get ready. That way, Daniel (Cheng) use the tooling bits >>>>>>> today, and you have as much time as you need to do the Right Thing upstream. >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Cheers, >>>>>>>> /Manuel >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> in upstream clang instead of having >>>>>>>>>> shadow versions in chrome. We have plans to upstream some of our >>>>>>>>>> >>>>>>>>> infrastructure >>>>>>>>> >>>>>>>>>> around large scale changes where we recently started to develop >>>>>>>>>> better >>>>>>>>>> abstractions to be able to plug in serialization etc, and you'll >>>>>>>>>> get >>>>>>>>>> clang-format integration and correct deduplication and >>>>>>>>>> application of changes >>>>>>>>>> for free. >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>>>>> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >>>>>>>>> >>>>>>>>>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>>>>> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >>>>>>>>> >>>>>>>>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch >>>>>>>>>> (mode_) { >>>>>>>>>> I'd use 3 classes instead of the switch. >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
Right, but chromium is generally an open-source project. Also, most chromium engineers aren't very familiar with google3, so some small self-contained local script is probably faster to get started with. (Also, this works on mac.) On Mon, Apr 1, 2013 at 10:28 AM, Daniel Jasper <djasper@google.com> wrote: > Last I checked, dcheng is a Googler :-). > > > On Mon, Apr 1, 2013 at 7:26 PM, Manuel Klimek <klimek@google.com> wrote: > >> On Mon, Apr 1, 2013 at 7:25 PM, Nico Weber <thakis@chromium.org> wrote: >> >>> On Mon, Apr 1, 2013 at 10:09 AM, Daniel Jasper <djasper@google.com>wrote: >>> >>>> Well, I had not taken a look at the recent changelist. However, it >>>> seems to me that most of what run_tool.py contains is the key part of the >>>> infrastructure that should be upstreamed. And that is also what I meant by >>>> "quite a bit to go". There is a significant amount of design and >>>> implementation that needs to be done before such tools become sane. >>>> >>>> I still don't fully understand the purpose of this tool. If it is meant >>>> as an example, we probably should get the infrastructure right, first. If >>>> you need this refactoring for Chrome now, why not use ClangMR? >>>> >>> >>> ClangMR is for googlers only, right? >>> >> >> Yep. >> >> >>> >>> >>>> Does this tool work on Windows? >>>> >>>> >>>> >>>> On Mon, Apr 1, 2013 at 6:53 PM, Manuel Klimek <klimek@google.com>wrote: >>>> >>>>> On Mon, Apr 1, 2013 at 6:50 PM, Nico Weber <thakis@chromium.org>wrote: >>>>> >>>>>> On Mon, Apr 1, 2013 at 9:46 AM, Daniel Jasper <djasper@google.com>wrote: >>>>>> >>>>>>> So, I might have gotten the earlier mail thread with Daniel wrong, >>>>>>> but what is the purpose of tools like this? From the earlier discussion I >>>>>>> gathered that the problem would be much better addressed with ClangMRs >>>>>>> running over the Chromium codebase. What's missing seem to be working >>>>>>> indexes of Windows and Mac compiles. >>>>>>> >>>>>>> If such a tool runs on an entire compilation database as Nico is >>>>>>> suggesting in one of the comments, this would take quite long (days?) >>>>>>> >>>>>> >>>>>> No, this should be at least as fast as building chrome, so it'd take >>>>>> less than an hour I think. dcheng can probably give you a concrete number. >>>>>> >>>>> >>>>> +1, I'd expect this to not take too long on one of the chrome dev >>>>> machines ;) >>>>> >>>>> >>>>>> >>>>>> >>>>>>> . While, I agree that we can upstream some of the stuff we have, I >>>>>>> think it is quite a bit to go until we have tools that can reasonably work >>>>>>> on a code base as large as Chrome's. >>>>>>> >>>>>>> >>>>>>> On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org>wrote: >>>>>>> >>>>>>>> On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com>wrote: >>>>>>>> >>>>>>>>> On Mon, Apr 1, 2013 at 6:33 PM, <thakis@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> On 2013/04/01 16:30:33, klimek wrote: >>>>>>>>>> >>>>>>>>>>> One of the things that happen when we're not fast enough on the >>>>>>>>>>> upstream track >>>>>>>>>>> :( I'd like to see things along those lines >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Do you mean a string("") -> string() tool? Or do you mean the >>>>>>>>>> python bits? >>>>>>>>> >>>>>>>>> >>>>>>>>> I mean: >>>>>>>>> 1. good examples on how to write tests upstream (without FileTest) >>>>>>>>> 2. an abstraction for handling lists of replacements, where >>>>>>>>> serialization and deserialization is plug-inable, and which is "applicable" >>>>>>>>> on a file >>>>>>>>> 3. a tool that slurps in a set of those lists of replacements and >>>>>>>>> applies them (note that you'll want more than replacements, for example >>>>>>>>> when you want to add a header) >>>>>>>>> >>>>>>>> >>>>>>>> Sounds like "the python bits". >>>>>>>> >>>>>>>> I don't think it's terrible that we have this script until the >>>>>>>> upstream bits get ready. That way, Daniel (Cheng) use the tooling bits >>>>>>>> today, and you have as much time as you need to do the Right Thing upstream. >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> Cheers, >>>>>>>>> /Manuel >>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> in upstream clang instead of having >>>>>>>>>>> shadow versions in chrome. We have plans to upstream some of our >>>>>>>>>>> >>>>>>>>>> infrastructure >>>>>>>>>> >>>>>>>>>>> around large scale changes where we recently started to develop >>>>>>>>>>> better >>>>>>>>>>> abstractions to be able to plug in serialization etc, and you'll >>>>>>>>>>> get >>>>>>>>>>> clang-format integration and correct deduplication and >>>>>>>>>>> application of changes >>>>>>>>>>> for free. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>>>>>> clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> >>>>>>>>>> >>>>>>>>>>> File tools/clang/empty_string/**EmptyStringConverter.cpp >>>>>>>>>>> (right): >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** >>>>>>>>>> clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> >>>>>>>>>> >>>>>>>>>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch >>>>>>>>>>> (mode_) { >>>>>>>>>>> I'd use 3 classes instead of the switch. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
On 2013/04/01 17:28:09, djasper_google.com wrote: > Last I checked, dcheng is a Googler :-). > > > On Mon, Apr 1, 2013 at 7:26 PM, Manuel Klimek <mailto:klimek@google.com> wrote: > > > On Mon, Apr 1, 2013 at 7:25 PM, Nico Weber <mailto:thakis@chromium.org> wrote: > > > >> On Mon, Apr 1, 2013 at 10:09 AM, Daniel Jasper <djasper@google.com>wrote: > >> > >>> Well, I had not taken a look at the recent changelist. However, it seems > >>> to me that most of what run_tool.py contains is the key part of the > >>> infrastructure that should be upstreamed. And that is also what I meant by > >>> "quite a bit to go". There is a significant amount of design and > >>> implementation that needs to be done before such tools become sane. > >>> > >>> I still don't fully understand the purpose of this tool. If it is meant > >>> as an example, we probably should get the infrastructure right, first. If > >>> you need this refactoring for Chrome now, why not use ClangMR? > >>> > >> > >> ClangMR is for googlers only, right? > >> > > > > Yep. > > > > > >> > >> > >>> Does this tool work on Windows? > >>> > >>> > >>> > >>> On Mon, Apr 1, 2013 at 6:53 PM, Manuel Klimek <mailto:klimek@google.com> wrote: > >>> > >>>> On Mon, Apr 1, 2013 at 6:50 PM, Nico Weber <mailto:thakis@chromium.org> wrote: > >>>> > >>>>> On Mon, Apr 1, 2013 at 9:46 AM, Daniel Jasper <djasper@google.com>wrote: > >>>>> > >>>>>> So, I might have gotten the earlier mail thread with Daniel wrong, > >>>>>> but what is the purpose of tools like this? From the earlier discussion > I > >>>>>> gathered that the problem would be much better addressed with ClangMRs > >>>>>> running over the Chromium codebase. What's missing seem to be working > >>>>>> indexes of Windows and Mac compiles. > >>>>>> > >>>>>> If such a tool runs on an entire compilation database as Nico is > >>>>>> suggesting in one of the comments, this would take quite long (days?) > >>>>>> > >>>>> > >>>>> No, this should be at least as fast as building chrome, so it'd take > >>>>> less than an hour I think. dcheng can probably give you a concrete > number. > >>>>> > >>>> > >>>> +1, I'd expect this to not take too long on one of the chrome dev > >>>> machines ;) > >>>> > >>>> > >>>>> > >>>>> > >>>>>> . While, I agree that we can upstream some of the stuff we have, I > >>>>>> think it is quite a bit to go until we have tools that can reasonably > work > >>>>>> on a code base as large as Chrome's. > >>>>>> > >>>>>> > >>>>>> On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org>wrote: > >>>>>> > >>>>>>> On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek <klimek@google.com>wrote: > >>>>>>> > >>>>>>>> On Mon, Apr 1, 2013 at 6:33 PM, <mailto:thakis@chromium.org> wrote: > >>>>>>>> > >>>>>>>>> On 2013/04/01 16:30:33, klimek wrote: > >>>>>>>>> > >>>>>>>>>> One of the things that happen when we're not fast enough on the > >>>>>>>>>> upstream track > >>>>>>>>>> :( I'd like to see things along those lines > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> Do you mean a string("") -> string() tool? Or do you mean the > >>>>>>>>> python bits? > >>>>>>>> > >>>>>>>> > >>>>>>>> I mean: > >>>>>>>> 1. good examples on how to write tests upstream (without FileTest) > >>>>>>>> 2. an abstraction for handling lists of replacements, where > >>>>>>>> serialization and deserialization is plug-inable, and which is > "applicable" > >>>>>>>> on a file > >>>>>>>> 3. a tool that slurps in a set of those lists of replacements and > >>>>>>>> applies them (note that you'll want more than replacements, for > example > >>>>>>>> when you want to add a header) > >>>>>>>> > >>>>>>> > >>>>>>> Sounds like "the python bits". > >>>>>>> > >>>>>>> I don't think it's terrible that we have this script until the > >>>>>>> upstream bits get ready. That way, Daniel (Cheng) use the tooling bits > >>>>>>> today, and you have as much time as you need to do the Right Thing > upstream. > >>>>>>> > >>>>>>> > >>>>>>>> > >>>>>>>> Cheers, > >>>>>>>> /Manuel > >>>>>>>> > >>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> in upstream clang instead of having > >>>>>>>>>> shadow versions in chrome. We have plans to upstream some of our > >>>>>>>>>> > >>>>>>>>> infrastructure > >>>>>>>>> > >>>>>>>>>> around large scale changes where we recently started to develop > >>>>>>>>>> better > >>>>>>>>>> abstractions to be able to plug in serialization etc, and you'll > >>>>>>>>>> get > >>>>>>>>>> clang-format integration and correct deduplication and > >>>>>>>>>> application of changes > >>>>>>>>>> for free. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** > >>>>>>>>> > clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> > >>>>>>>>> > >>>>>>>>>> File tools/clang/empty_string/**EmptyStringConverter.cpp (right): > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> https://codereview.chromium.**org/12746010/diff/48001/tools/** > >>>>>>>>> > clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> > >>>>>>>>> > >>>>>>>>>> tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch > >>>>>>>>>> (mode_) { > >>>>>>>>>> I'd use 3 classes instead of the switch. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > https://codereview.chromium.**org/12746010/%3Chttps://codereview.chromium.org...> > >>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > I am indeed a Googler. However, from the short thread I started last time, my impression is it'd be some time before the refactoring tools were upstreamed fully into Clang. Until then, there's still a few large refactoring tasks (scoped_ptr related changes mostly, but there's also some namespace cleanups) that we'd like to do. Since ClangMR does not fully satisfy these use cases (it doesn't work on Windows, Mac, Android configurations, etc), we'd like a tool in the interim that helps us with these project-wide changes. This tool does /not/ yet work on Windows, but it does work on Linux and Mac, and getting it to work decently on Windows would be my next goal. Also, this tool takes about 30 minutes to run on my T3500 which has 8 cores (with hyperthreading). With a Chrome dev machine, I'd expect it to run 2x-4x faster.
Nico: I know that there are a lot of open-source developers. Hence my question regarding the purpose of this tool. IMO, it should not be used as an example / copy&paste source for others. Now, the problem is that we are probably all very limited on available cycles, which makes it even more important to work on the right things. So, if we want to work on an example refactoring tool for Chrome, I suggest that we meet up and discuss how to approach this, come up with a design and distribute tasks for implementation. As Manuel said, we have a fair bit that should be fairly easy to upstream and there are the new bits, e.g. what run_tool.py does, which would also find a nice home in LibTooling. If you want this and a few other refactorings now, I think cycles might be best spent on making ClangMR work for the other platforms (which would be a tremendous win for a lot of people). Mac should be easy. If you can make the tool work on Windows, I don't see a reason why we shouldn't be able to index it. I don't exactly know what Android will entail. I don't want to sound too negative. I think the stuff in the CL is awesome and the fact that you can run it over the entire Chromium code in 10-15mins even more so. But that makes me want to put even more energy into getting it right. On Mon, Apr 1, 2013 at 7:33 PM, <dcheng@chromium.org> wrote: > On 2013/04/01 17:28:09, djasper_google.com wrote: > >> Last I checked, dcheng is a Googler :-). >> > > > On Mon, Apr 1, 2013 at 7:26 PM, Manuel Klimek <mailto:klimek@google.com> >> > wrote: > > > On Mon, Apr 1, 2013 at 7:25 PM, Nico Weber <mailto:thakis@chromium.org> >> > wrote: > >> > >> >> On Mon, Apr 1, 2013 at 10:09 AM, Daniel Jasper <djasper@google.com >> >wrote: >> >> >> >>> Well, I had not taken a look at the recent changelist. However, it >> seems >> >>> to me that most of what run_tool.py contains is the key part of the >> >>> infrastructure that should be upstreamed. And that is also what I >> meant by >> >>> "quite a bit to go". There is a significant amount of design and >> >>> implementation that needs to be done before such tools become sane. >> >>> >> >>> I still don't fully understand the purpose of this tool. If it is >> meant >> >>> as an example, we probably should get the infrastructure right, >> first. If >> >>> you need this refactoring for Chrome now, why not use ClangMR? >> >>> >> >> >> >> ClangMR is for googlers only, right? >> >> >> > >> > Yep. >> > >> > >> >> >> >> >> >>> Does this tool work on Windows? >> >>> >> >>> >> >>> >> >>> On Mon, Apr 1, 2013 at 6:53 PM, Manuel Klimek <mailto: >> klimek@google.com> >> > wrote: > >> >>> >> >>>> On Mon, Apr 1, 2013 at 6:50 PM, Nico Weber <mailto: >> thakis@chromium.org> >> > wrote: > >> >>>> >> >>>>> On Mon, Apr 1, 2013 at 9:46 AM, Daniel Jasper <djasper@google.com >> >wrote: >> >>>>> >> >>>>>> So, I might have gotten the earlier mail thread with Daniel wrong, >> >>>>>> but what is the purpose of tools like this? From the earlier >> discussion >> I >> >>>>>> gathered that the problem would be much better addressed with >> ClangMRs >> >>>>>> running over the Chromium codebase. What's missing seem to be >> working >> >>>>>> indexes of Windows and Mac compiles. >> >>>>>> >> >>>>>> If such a tool runs on an entire compilation database as Nico is >> >>>>>> suggesting in one of the comments, this would take quite long >> (days?) >> >>>>>> >> >>>>> >> >>>>> No, this should be at least as fast as building chrome, so it'd take >> >>>>> less than an hour I think. dcheng can probably give you a concrete >> number. >> >>>>> >> >>>> >> >>>> +1, I'd expect this to not take too long on one of the chrome dev >> >>>> machines ;) >> >>>> >> >>>> >> >>>>> >> >>>>> >> >>>>>> . While, I agree that we can upstream some of the stuff we have, I >> >>>>>> think it is quite a bit to go until we have tools that can >> reasonably >> work >> >>>>>> on a code base as large as Chrome's. >> >>>>>> >> >>>>>> >> >>>>>> On Mon, Apr 1, 2013 at 6:41 PM, Nico Weber <thakis@chromium.org >> >wrote: >> >>>>>> >> >>>>>>> On Mon, Apr 1, 2013 at 9:38 AM, Manuel Klimek >> > <klimek@google.com>wrote: > >> >>>>>>> >> >>>>>>>> On Mon, Apr 1, 2013 at 6:33 PM, <mailto:thakis@chromium.org> >> wrote: >> >>>>>>>> >> >>>>>>>>> On 2013/04/01 16:30:33, klimek wrote: >> >>>>>>>>> >> >>>>>>>>>> One of the things that happen when we're not fast enough on the >> >>>>>>>>>> upstream track >> >>>>>>>>>> :( I'd like to see things along those lines >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> Do you mean a string("") -> string() tool? Or do you mean the >> >>>>>>>>> python bits? >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> I mean: >> >>>>>>>> 1. good examples on how to write tests upstream (without >> FileTest) >> >>>>>>>> 2. an abstraction for handling lists of replacements, where >> >>>>>>>> serialization and deserialization is plug-inable, and which is >> "applicable" >> >>>>>>>> on a file >> >>>>>>>> 3. a tool that slurps in a set of those lists of replacements and >> >>>>>>>> applies them (note that you'll want more than replacements, for >> example >> >>>>>>>> when you want to add a header) >> >>>>>>>> >> >>>>>>> >> >>>>>>> Sounds like "the python bits". >> >>>>>>> >> >>>>>>> I don't think it's terrible that we have this script until the >> >>>>>>> upstream bits get ready. That way, Daniel (Cheng) use the tooling >> bits >> >>>>>>> today, and you have as much time as you need to do the Right Thing >> upstream. >> >>>>>>> >> >>>>>>> >> >>>>>>>> >> >>>>>>>> Cheers, >> >>>>>>>> /Manuel >> >>>>>>>> >> >>>>>>>> >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> in upstream clang instead of having >> >>>>>>>>>> shadow versions in chrome. We have plans to upstream some of >> our >> >>>>>>>>>> >> >>>>>>>>> infrastructure >> >>>>>>>>> >> >>>>>>>>>> around large scale changes where we recently started to develop >> >>>>>>>>>> better >> >>>>>>>>>> abstractions to be able to plug in serialization etc, and >> you'll >> >>>>>>>>>> get >> >>>>>>>>>> clang-format integration and correct deduplication and >> >>>>>>>>>> application of changes >> >>>>>>>>>> for free. >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> https://codereview.chromium.****org/12746010/diff/48001/tools/* >> *** >> >>>>>>>>> >> > > clang/empty_string/****EmptyStringConverter.cpp<https** > ://codereview.chromium.org/**12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> > > > >> >>>>>>>>> >> >>>>>>>>>> File tools/clang/empty_string/****EmptyStringConverter.cpp >> (right): >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> https://codereview.chromium.****org/12746010/diff/48001/tools/* >> *** >> >>>>>>>>> >> > > clang/empty_string/****EmptyStringConverter.cpp#****newcode103< > https://codereview.**chromium.org/12746010/diff/**48001/tools/clang/empty_ > **string/EmptyStringConverter.**cpp#newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> > > > >> >>>>>>>>> >> >>>>>>>>>> tools/clang/empty_string/****EmptyStringConverter.cpp:103: >> switch >> >> >>>>>>>>>> (mode_) { >> >>>>>>>>>> I'd use 3 classes instead of the switch. >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> >> > > https://codereview.chromium.****org/12746010/%3Chttps://codere** > view.chromium.org/12746010/ <http://codereview.chromium.org/12746010/>> > >> >>>>>>>>> >> >>>>>>>> >> >>>>>>>> >> >>>>>>> >> >>>>>> >> >>>>> >> >>>> >> >>> >> >> >> > >> > > I am indeed a Googler. However, from the short thread I started last time, > my > impression is it'd be some time before the refactoring tools were > upstreamed > fully into Clang. Until then, there's still a few large refactoring tasks > (scoped_ptr related changes mostly, but there's also some namespace > cleanups) > that we'd like to do. Since ClangMR does not fully satisfy these use cases > (it > doesn't work on Windows, Mac, Android configurations, etc), we'd like a > tool in > the interim that helps us with these project-wide changes. This tool does > /not/ > yet work on Windows, but it does work on Linux and Mac, and getting it to > work > decently on Windows would be my next goal. > > Also, this tool takes about 30 minutes to run on my T3500 which has 8 cores > (with hyperthreading). With a Chrome dev machine, I'd expect it to run > 2x-4x > faster. > > https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >
I'm working on adding tests as well. I'll upload those in a bit. To try to answer some of Daniel's concerns: On 2013/04/01 18:01:08, djasper_google.com wrote: > Nico: I know that there are a lot of open-source developers. Hence my > question regarding the purpose of this tool. IMO, it should not be used as > an example / copy&paste source for others. > Chrome developers already have need for this kind of tool, and it helps fill a gap in the current tools. Several examples: - rsleevi@ recently hacked the Chrome plugin to rewrite files for a cleanup CL he was working on. I have done similar things myself in the past. - brettw@ has been doing a lot of work cleaning up the base namespace in Chrome. As far as I'm aware, this is mostly a manual process using find and replace text. - A year ago, a bunch of Chrome devs spent a non-trivial amount of time hand-converting code to use the new callback system. All these would be easier with tooling support (and reduce duplicated work), which is what this patch aims to provide. Perhaps there's a better long-term approach, but it's good enough for now, which is what's important. > Now, the problem is that we are probably all very limited on available > cycles, which makes it even more important to work on the right things. So, > if we want to work on an example refactoring tool for Chrome, I suggest > that we meet up and discuss how to approach this, come up with a design and > distribute tasks for implementation. As Manuel said, we have a fair bit > that should be fairly easy to upstream and there are the new bits, e.g. > what run_tool.py does, which would also find a nice home in LibTooling. > I think this would be good to have. But I don't think we should require this patch to depend on that meeting. > If you want this and a few other refactorings now, I think cycles might be > best spent on making ClangMR work for the other platforms (which would be a > tremendous win for a lot of people). Mac should be easy. If you can make > the tool work on Windows, I don't see a reason why we shouldn't be able to > index it. I don't exactly know what Android will entail. > The current flow I'm using is pretty far removed from the ClangMR flow (unfortunately). I don't have any notion of what would be involved > I don't want to sound too negative. I think the stuff in the CL is awesome > and the fact that you can run it over the entire Chromium code in 10-15mins > even more so. But that makes me want to put even more energy into getting > it right. I agree that we should get it right so we can upstream the right approach. But perhaps consider this more of a learning experience and experiment for Chrome. =) https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... File tools/clang/empty_string/EmptyStringConverter.cpp (right): https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:4: On 2013/04/01 16:19:35, Nico wrote: > Add a comment somewhere that gives a short overview of what this does Done. https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:41: kTemporaryMode, On 2013/04/01 16:19:35, Nico wrote: > nit: trailing commas in enums are a c++11 feature And now it's gone =) https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:46: EmptyStringConverterCallback(ReplacementMode mode, Replacements* replacements) On 2013/04/01 16:19:35, Nico wrote: > Maybe you want to use llvm style instead of chromium style for this? (in that > case, "type *varname", or pass -style LLVM to clang-format) It's hard to say. The Chrome plugin uses Chromium coding style but names its files using LLVM style. I opted to follow the same approach here. https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:49: virtual void run(const MatchFinder::MatchResult& result); On 2013/04/01 16:19:35, Nico wrote: > There's LLVM_OVERRIDE I think (which expands to nothing in c++98 mode, but it > does serve as documentation at least for now) Done. https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:72: const auto& constructor_call = On 2013/04/01 16:19:35, Nico wrote: > Also, since this is meant to be a demo, maybe add a short comment that explains > how to read matchers (or, better yet, include a link to the matcher docs) This was originally meant to be a demo... but there seems to be some support for actually landing the diffs generated by this (it does result in a 14-18 KB reduction in the generated binary). I will update the preliminary documentation at https://code.google.com/p/chromium/wiki/ClangToolRefactoring to cover this. https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:103: switch (mode_) { On 2013/04/01 16:30:33, klimek wrote: > I'd use 3 classes instead of the switch. Done.
On Mon, Apr 1, 2013 at 10:11 PM, <dcheng@chromium.org> wrote: > I'm working on adding tests as well. I'll upload those in a bit. > > To try to answer some of Daniel's concerns: > > On 2013/04/01 18:01:08, djasper_google.com wrote: > >> Nico: I know that there are a lot of open-source developers. Hence my >> question regarding the purpose of this tool. IMO, it should not be used as >> an example / copy&paste source for others. >> > > > Chrome developers already have need for this kind of tool, and it helps > fill a > gap in the current tools. Several examples: > - rsleevi@ recently hacked the Chrome plugin to rewrite files for a > cleanup CL > he was working on. I have done similar things myself in the past. > - brettw@ has been doing a lot of work cleaning up the base namespace in > Chrome. > As far as I'm aware, this is mostly a manual process using find and replace > text. > - A year ago, a bunch of Chrome devs spent a non-trivial amount of time > hand-converting code to use the new callback system. > All these would be easier with tooling support (and reduce duplicated > work), > which is what this patch aims to provide. Perhaps there's a better > long-term > approach, but it's good enough for now, which is what's important. Right. Just to clarify. I think we can/should/will come up with a good solution in a matter of weeks (not months/quarters!). So, I'd still be hesitant to suggest using this patch as example of how to do it. But I guess, we can easily migrate other tools if people start copying this flow. > Now, the problem is that we are probably all very limited on available >> cycles, which makes it even more important to work on the right things. >> So, >> if we want to work on an example refactoring tool for Chrome, I suggest >> that we meet up and discuss how to approach this, come up with a design >> and >> distribute tasks for implementation. As Manuel said, we have a fair bit >> that should be fairly easy to upstream and there are the new bits, e.g. >> what run_tool.py does, which would also find a nice home in LibTooling. >> > > > I think this would be good to have. But I don't think we should require > this > patch to depend on that meeting. What I am saying is not really related to this patch. By all means, feel free to go through with it, especially as a lot of work has already gone into it and it definitely is a good proof-of-concept, no matter how we proceed. I just foresee that you'll encounter many of the issues that we have already faced in the past. Examples are how to include newly required headers, how to deal with duplicate or overlapping edits, how to re-format exactly the changed bits of code, ... I think before you start spending more time on solving those problems, we should try to use a shorter path to the long-term solution. As I said above, I think a first version of a good solution might be doable in few weeks (maybe just 1 or 2) if we combine efforts and reuse the stuff we already have. > > > If you want this and a few other refactorings now, I think cycles might be >> best spent on making ClangMR work for the other platforms (which would be >> a >> tremendous win for a lot of people). Mac should be easy. If you can make >> the tool work on Windows, I don't see a reason why we shouldn't be able to >> index it. I don't exactly know what Android will entail. >> > > > The current flow I'm using is pretty far removed from the ClangMR flow > (unfortunately). I don't have any notion of what would be involved > > I don't want to sound too negative. I think the stuff in the CL is awesome >> and the fact that you can run it over the entire Chromium code in >> 10-15mins >> even more so. But that makes me want to put even more energy into getting >> it right. >> > > I agree that we should get it right so we can upstream the right approach. > But > perhaps consider this more of a learning experience and experiment for > Chrome. > =) Will do :-). > > > > https://codereview.chromium.**org/12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp> > File tools/clang/empty_string/**EmptyStringConverter.cpp (right): > > https://codereview.chromium.**org/12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp#**newcode4<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode4> > tools/clang/empty_string/**EmptyStringConverter.cpp:4: > On 2013/04/01 16:19:35, Nico wrote: > >> Add a comment somewhere that gives a short overview of what this does >> > > Done. > > https://codereview.chromium.**org/12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp#**newcode41<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode41> > tools/clang/empty_string/**EmptyStringConverter.cpp:41: kTemporaryMode, > On 2013/04/01 16:19:35, Nico wrote: > >> nit: trailing commas in enums are a c++11 feature >> > > And now it's gone =) > > https://codereview.chromium.**org/12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp#**newcode46<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode46> > tools/clang/empty_string/**EmptyStringConverter.cpp:46: > EmptyStringConverterCallback(**ReplacementMode mode, Replacements* > replacements) > On 2013/04/01 16:19:35, Nico wrote: > >> Maybe you want to use llvm style instead of chromium style for this? >> > (in that > >> case, "type *varname", or pass -style LLVM to clang-format) >> > > It's hard to say. The Chrome plugin uses Chromium coding style but names > its files using LLVM style. I opted to follow the same approach here. > > https://codereview.chromium.**org/12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp#**newcode49<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode49> > tools/clang/empty_string/**EmptyStringConverter.cpp:49: virtual void > run(const MatchFinder::MatchResult& result); > On 2013/04/01 16:19:35, Nico wrote: > >> There's LLVM_OVERRIDE I think (which expands to nothing in c++98 mode, >> > but it > >> does serve as documentation at least for now) >> > > Done. > > https://codereview.chromium.**org/12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp#**newcode72<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode72> > tools/clang/empty_string/**EmptyStringConverter.cpp:72: const auto& > constructor_call = > On 2013/04/01 16:19:35, Nico wrote: > >> Also, since this is meant to be a demo, maybe add a short comment that >> > explains > >> how to read matchers (or, better yet, include a link to the matcher >> > docs) > > This was originally meant to be a demo... but there seems to be some > support for actually landing the diffs generated by this (it does result > in a 14-18 KB reduction in the generated binary). > > I will update the preliminary documentation at > https://code.google.com/p/**chromium/wiki/**ClangToolRefactoring<https://code... cover > this. > > > https://codereview.chromium.**org/12746010/diff/48001/tools/** > clang/empty_string/**EmptyStringConverter.cpp#**newcode103<https://codereview.chromium.org/12746010/diff/48001/tools/clang/empty_string/EmptyStringConverter.cpp#newcode103> > tools/clang/empty_string/**EmptyStringConverter.cpp:103: switch (mode_) { > On 2013/04/01 16:30:33, klimek wrote: > >> I'd use 3 classes instead of the switch. >> > > Done. > > https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >
PTAL. The big changes: - Updated list element deletion logic. The previous version incorrectly handled deletions that only deleted part of a list node (and therefore shouldn't have triggered the deletion extension logic). - A new test harness. - Got rid of the ugly [6:] hack in run_tool.py and normalized the paths by using os.path.relpath().
lgtm! https://codereview.chromium.org/12746010/diff/74002/tools/clang/empty_string/... File tools/clang/empty_string/EmptyStringConverter.cpp (right): https://codereview.chromium.org/12746010/diff/74002/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:8: // should be run using the tools/clang/scripts/run_tool.py helper. Either this or run_tool.py should have an example command line at the top. ("Use like this: $command")
Message was sent while issue was closed.
Committed patchset #19 manually as r191936 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/12746010/diff/82002/tools/clang/empty_string/... File tools/clang/empty_string/EmptyStringConverter.cpp (right): https://codereview.chromium.org/12746010/diff/82002/tools/clang/empty_string/... tools/clang/empty_string/EmptyStringConverter.cpp:99: const auto& constructor_call = Unfortunatelly I got the following error when running this command line: $ tools/clang/scripts/update.sh --force-local-build --without-android --with-tools-extra --with-chrome-tools "plugins empty_string" /home/tfarina/chromium/src/third_party/llvm/tools/clang/tools/chrome-empty_string/EmptyStringConverter.cpp:99:9: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions] const auto& constructor_call = ^
Message was sent while issue was closed.
On 2013/04/08 01:17:37, tfarina wrote: > https://codereview.chromium.org/12746010/diff/82002/tools/clang/empty_string/... > File tools/clang/empty_string/EmptyStringConverter.cpp (right): > > https://codereview.chromium.org/12746010/diff/82002/tools/clang/empty_string/... > tools/clang/empty_string/EmptyStringConverter.cpp:99: const auto& > constructor_call = > Unfortunatelly I got the following error when running this command line: > > $ tools/clang/scripts/update.sh --force-local-build --without-android > --with-tools-extra --with-chrome-tools "plugins empty_string" > > /home/tfarina/chromium/src/third_party/llvm/tools/clang/tools/chrome-empty_string/EmptyStringConverter.cpp:99:9: > warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions] > const auto& constructor_call = > ^ It's just a warning. I've been ignoring it =) FWIW, I'm planning on landing the results of this tool in https://codereview.chromium.org/13145003/... hopefully it'll land some time tomorrow night.
Message was sent while issue was closed.
Should we add this to chrome-plugin? Otherwise people will keep using it: https://codereview.chromium.org/14146006/diff/1/chrome/browser/sync_file_syst...
I don't think that's a problem. From what I understand this is just a proof of concept tool and not something that's terribly useful in itself (rewriting all existing instances saved something like 10kB). On Thu, Apr 18, 2013 at 6:24 PM, <tfarina@chromium.org> wrote: > Should we add this to chrome-plugin? > > Otherwise people will keep using it: > > https://codereview.chromium.**org/14146006/diff/1/chrome/** > browser/sync_file_system/**drive_file_sync_client.cc#**newcode188<https://codereview.chromium.org/14146006/diff/1/chrome/browser/sync_file_system/drive_file_sync_client.cc#newcode188> > > https://codereview.chromium.**org/12746010/<https://codereview.chromium.org/1... >
Message was sent while issue was closed.
On 2013/04/19 02:41:02, Nico wrote: > I don't think that's a problem. From what I understand this is just a proof > of concept tool and not something that's terribly useful in itself > (rewriting all existing instances saved something like 10kB). > > > On Thu, Apr 18, 2013 at 6:24 PM, <mailto:tfarina@chromium.org> wrote: > > > Should we add this to chrome-plugin? > > > > Otherwise people will keep using it: > > > > https://codereview.chromium.**org/14146006/diff/1/chrome/** > > > browser/sync_file_system/**drive_file_sync_client.cc#**newcode188<https://codereview.chromium.org/14146006/diff/1/chrome/browser/sync_file_system/drive_file_sync_client.cc#newcode188> > > > > > https://codereview.chromium.**org/12746010/%3Chttps://codereview.chromium.org...> > > When I have spare cycles, I'll look into adapting it for the clang plugin. A couple people expressed interest in this in https://codereview.chromium.org/13145003. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
