[rust] Cover `rustflags` / `--cfg` in `buildflag_header`. [chromium/src : main]
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 4 comments![]( "Open in Gerrit")
@dcheng, can you PTAL again?
I've addressed the comments (adding explicit test coverage of `true` and `false`). I also rebased this CL on top of ToT (i.e. making this independent of `--check-cfg`-related stuff).
FWIW, @dloehr is asking for similar functionality in https://crbug.com/462501862#comment11.
if (_value == "true" || _value == "1") {Łukasz AnforowiczMaybe we should note that this is the logic used by build/write_buildflag_header.py and/or have some test case that covers this.
`build/rust/tests/test_buildflag_header/BUILD.gn` is the test for covering this.
#[cfg(RUST_ALLOCATOR_USES_ALLOCATOR_IMPLS_H)]Łukasz AnforowiczIs the rust convention to use underscore_case? Do we want to do the same for buildflags? I guess that's maybe a bit harder in pure GN?
This is indeed hard to do in pure GN.
And in general, I think we try to avoid automatically changing identifier styles when they are used in other languages.
I think it's fine to land as-is, and in the future we may consider **adding** an alternative lowercase spelling (if we decide this is more ergonomic/expected + if we are ok with GN situation - either adding `string_lowercase` to GN built-in functions, or doing this with [gasp!] `string_replace` repeated 26 times).
is_bar = 1 == 2Łukasz AnforowiczOh I guess this is the test. Do we want to test explicit `true` / `false` at all?
Oh I guess this is the test.
Yes.
Do we want to test explicit `true` / `false` at all?
Done.
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
I guess there is an open question about potential naming conflicts. Maybe `build/buildflag_header.gni`-originated `--cfg` flags should have a `BUILDFLAG_` or `CHROMIUM_` prefix to make conflicts less likely? I note that the convention of spelling Chromium buildflags using SHOUTY_CASE makes it less likely to conflict with typical hacker_case_spelling of other Rust cfg flags). So, I think landing as-is should also be ok?
(In theory, instead of doing `--cfg=IS_ASAN` we can also do `'--cfg=buildflag="IS_ASAN"'`. It seems to me that the former is simpler + less verbose and just as good. For the latter it may be a tiny bit simpler to single to `cxxbridge` which flags are possible vs set [since `buildflag` is always "possible"], but it's a tiny difference.)
Related details
- Daniel Cheng
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. |