Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820433007:
Remove a TODO from components/spellcheck/renderer/BUILD.gn (Closed)
DescriptionRemove a TODO from components/spellcheck/renderer/BUILD.gn
BUG=711509
Review-Url: https://codereview.chromium.org/2820433007
Cr-Commit-Position: refs/heads/master@{#464796}
Committed: https://chromium.googlesource.com/chromium/src/+/af00eab7c9c6666be2a8c6876280df4fbdb9c655
Patch Set 1 #
Messages
Total messages: 17 (7 generated)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaochengh@chromium.org changed reviewers: + rouslan@chromium.org, thestig@chromium.org
PTAL. I think it's better not to change the way of how source set is set, because: The whole file should use the same pattern to include source files. Either include-all followed by exclusion, or include-common followed by inclusion. For the unit_tests source set (in the same file), there are some files (e.g., spellcheck_multilingual_unittest.cc) that are not included on Mac or Android. Changing it to include-common followed by inclusion makes the file messy. So we should keep this file as is.
lgtm
The CQ bit was unchecked by xiaochengh@chromium.org
The CQ bit was checked by xiaochengh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1492202635550210, "parent_rev":
"3e9c794b9ff12579e3f658dc62327fc63bbb73d5", "commit_rev":
"af00eab7c9c6666be2a8c6876280df4fbdb9c655"}
Message was sent while issue was closed.
Description was changed from ========== Remove a TODO from components/spellcheck/renderer/BUILD.gn BUG=711509 ========== to ========== Remove a TODO from components/spellcheck/renderer/BUILD.gn BUG=711509 Review-Url: https://codereview.chromium.org/2820433007 Cr-Commit-Position: refs/heads/master@{#464796} Committed: https://chromium.googlesource.com/chromium/src/+/af00eab7c9c6666be2a8c6876280... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/af00eab7c9c6666be2a8c6876280...
Message was sent while issue was closed.
On 2017/04/14 20:17:47, Xiaocheng wrote: > PTAL. > > I think it's better not to change the way of how source set is set, because: > > The whole file should use the same pattern to include source files. Either > include-all followed by exclusion, or include-common followed by inclusion. Yes, it should. Not just one conditional block. The entire file. > For the unit_tests source set (in the same file), there are some files (e.g., > spellcheck_multilingual_unittest.cc) that are not included on Mac or Android. > Changing it to include-common followed by inclusion makes the file messy. Wait, it does? > So we should keep this file as is. I'll make a CL then. It'll end up reducing duplication.
Message was sent while issue was closed.
On 2017/04/15 at 04:27:09, thestig wrote:
> On 2017/04/14 20:17:47, Xiaocheng wrote:
> > PTAL.
> >
> > I think it's better not to change the way of how source set is set, because:
> >
> > The whole file should use the same pattern to include source files. Either
> > include-all followed by exclusion, or include-common followed by inclusion.
>
> Yes, it should. Not just one conditional block. The entire file.
>
> > For the unit_tests source set (in the same file), there are some files
(e.g.,
> > spellcheck_multilingual_unittest.cc) that are not included on Mac or
Android.
> > Changing it to include-common followed by inclusion makes the file messy.
>
> Wait, it does?
>
> > So we should keep this file as is.
>
> I'll make a CL then. It'll end up reducing duplication.
It seems messy to have
sources = [...]
if (!is_android && !is_mac) {
sources += [...]
}
if (!is_android) {
sources += [...]
}
The number of terms in the if condition and the number of if statements may grow
larger if we have more build flags.
Anyway, I'm wondering if there is a style guide for BUILD.gn files.
Message was sent while issue was closed.
On 2017/04/15 04:44:25, Xiaocheng wrote: > It seems messy to have See https://codereview.chromium.org/2820873003 - it reduces the number of lines. Given a file, you know exactly under what conditions it will be included in the build. With the current model, the mental model is instead, "it will be built, unless $cond_a, or $cond_b, or ..." > The number of terms in the if condition and the number of if statements may grow > larger if we have more build flags. It can grow either way. Usually one uses a build flag so that alone is the conditional on which something is gated, so the number of terms in the if condition should remain close to 1. > Anyway, I'm wondering if there is a style guide for BUILD.gn files. Most of the other components/spellcheck GN files are inclusive. So are the deps entries in this file.
Message was sent while issue was closed.
On 2017/04/15 at 04:51:35, thestig wrote: > On 2017/04/15 04:44:25, Xiaocheng wrote: > > It seems messy to have > > See https://codereview.chromium.org/2820873003 - it reduces the number of lines. Given a file, you know exactly under what conditions it will be included in the build. With the current model, the mental model is instead, "it will be built, unless $cond_a, or $cond_b, or ..." > > > The number of terms in the if condition and the number of if statements may grow > > larger if we have more build flags. > > It can grow either way. Usually one uses a build flag so that alone is the conditional on which something is gated, so the number of terms in the if condition should remain close to 1. > > > Anyway, I'm wondering if there is a style guide for BUILD.gn files. > > Most of the other components/spellcheck GN files are inclusive. So are the deps entries in this file. Maybe I still need to improve my taste in coding style... With the inclusive pattern, we lose the 1:1 relationship of which flag controls which files. Anyway, I don't have a strong preference. I'm fine as long as an owner thinks it's good.
Message was sent while issue was closed.
On 2017/04/15 05:25:21, Xiaocheng wrote: > Maybe I still need to improve my taste in coding style... With the inclusive > pattern, we lose the 1:1 relationship of which flag controls which files. I'm not sure why you say that. Using spellcheck_multilingual_unittest.cc as an example, right now it appears in 3 places in the file and one has to read 2 separate conditionals to understand when it gets built. With the inclusive model, it only appears once and is gated on 1 conditional. Isn't that more "1:1" ? Another way to thing about it is to align the the GN file with C++ #includes. After all, in C++, one does not #include everything, and then #exclude some headers.
Message was sent while issue was closed.
On 2017/04/15 at 05:58:22, thestig wrote: > On 2017/04/15 05:25:21, Xiaocheng wrote: > > Maybe I still need to improve my taste in coding style... With the inclusive > > pattern, we lose the 1:1 relationship of which flag controls which files. > > I'm not sure why you say that. Using spellcheck_multilingual_unittest.cc as an example, right now it appears in 3 places in the file and one has to read 2 separate conditionals to understand when it gets built. With the inclusive model, it only appears once and is gated on 1 conditional. Isn't that more "1:1" ? > > Another way to thing about it is to align the the GN file with C++ #includes. After all, in C++, one does not #include everything, and then #exclude some headers. That makes sense. Thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
