Teach make_dafsa.py to generate C++ within a namespace [chromium/src : main]
Chris Fredrickson (Gerrit)
Chris Fredrickson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chris Fredrickson
| Commit-Queue | +1 |
1 comment
CC Stefano since I know this rearranges some things in https://crrev.com/c/7134745. Since only one of these gperf files is generated, my hope is that splitting these actions apart actually makes it easier to deal with the generated gperf file differently if we have to.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson added 1 comment![]( "Open in Gerrit")
+meacer for components/lookalikes and components/url_formatter
Related details
- Adam Rice
- Mustafa Emre Acer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 3 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
3 comments
lgtm but you might want to get another reviewer for the BUILD files if you want them checker harder.
net::LookupStringInFixedSet(net::registry_controlled_domains::kDafsa,Not required: If you happen to do another patch set,
`base::as_string_buffer(UNSAFE_BUFFERS(base::span(data, size)))` would be a nicer way to write this.
_gperf_files = [I don't have a good understanding of GN files, so I'm assuming this is the best way to do this.
Related details
- Chris Fredrickson
- Mustafa Emre Acer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +0 |
1 comment
foreach(_item, _gperf_files) {Gemini pointed out that the two foreach loops could be combined into one. It might be worth considering.
Related details
- Chris Fredrickson
- Mustafa Emre Acer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson added 3 comments![]( "Open in Gerrit")
net::LookupStringInFixedSet(net::registry_controlled_domains::kDafsa,Not required: If you happen to do another patch set,
`base::as_string_buffer(UNSAFE_BUFFERS(base::span(data, size)))` would be a nicer way to write this.
I'm not familiar with `base::as_string_buffer` and codesearch is coming up empty, is that the right spelling?
I don't have a good understanding of GN files, so I'm assuming this is the best way to do this.
Ack. I'm not an expert either, but I see similar code following this pattern in other gn files. And `action_foreach` only loops over a list of sources; it doesn't allow looping over a list of tuples.
Gemini pointed out that the two foreach loops could be combined into one. It might be worth considering.
- Adam Rice
- Mustafa Emre Acer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustafa Emre Acer (Gerrit)
Mustafa Emre Acer voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mustafa Emre Acer
| Code-Review | +1 |
2 comments
"data/common_words.gperf",Q: Could this also be a tuple? It seems simpler, but I'm not a build expert:
action_foreach("common_words_dafsa") {
script = "//net/tools/dafsa/make_dafsa.py"
sources = [
("data/common_words.gperf", "url_formatter::common_words"),
...
]
outputs = [ "${target_gen_dir}/{{source_name_part}}-inc.cc" ]
args = [
"--namespace=${source[1]}",
"{{source[0]}}",
rebase_path("${target_gen_dir}/{{source_name_part}}-inc.cc",
root_build_dir),
]
}Related details
- Adam Rice
- Chris Fredrickson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson added 1 comment![]( "Open in Gerrit")
"data/common_words.gperf",Q: Could this also be a tuple? It seems simpler, but I'm not a build expert:
action_foreach("common_words_dafsa") {
script = "//net/tools/dafsa/make_dafsa.py"
sources = [
("data/common_words.gperf", "url_formatter::common_words"),
...
]
outputs = [ "${target_gen_dir}/{{source_name_part}}-inc.cc" ]
args = [
"--namespace=${source[1]}",
"{{source[0]}}",
rebase_path("${target_gen_dir}/{{source_name_part}}-inc.cc",
root_build_dir),
]
}
Unfortunately no, action_foreach only iterates over strings, not tuples of any kind. In order to make this CL work, we have to refactor to use `action` and `foreach` instead of `action_foreach`, since `foreach` can iterate over lists of tuples.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |