Create a bcp47 constexpr parser to be used in LanguageTag. [chromium/src : main]
Greg Thompson (Gerrit)
Greg Thompson added 12 comments![]( "Open in Gerrit")
constexpr std::optional<ParsedBcp47Tag> ParseBcp47Tag(std::string_view tag) {`std::string_view tag LIFETIME_BOUND`
base::span<const std::string_view> subtags) {`base::span<const std::string_view> subtags LIFETIME_BOUND`
(#include "base/compiler_specific.h")
// See the comments in `IsLanguageSubgtag`.Please fix this WARNING reported by autoreview issue finding: There are a few typos in these comments:
- `IsLanguageSubgtag` -> `IsLanguageSubtag`
- `IsScriptSubgtag` -> `IsScriptSubtag`
- `IsRegionSubgtag` -> `IsRegion` (the function is named `IsRegion`)
- `IsVariantSubgtag` -> `IsVariant` (the function is named `IsVariant`)
// The parsed BCP47 tag. It is a view on the actual input string, be-aware of
// life-time.i find this wording a little odd. consider removing this since we can use an annotation to enforce the correct behavior; see below.
return std::ranges::all_of(#include <algorithm>
namespace base::i18n::internal {i recently became aware of the guidance to avoid generic nested namespace named like `internal` in go/totw/130#best-practices. wdyt about `base::i18n_internal` here? can migrate other files in a distinct CL.
#include "base/containers/fixed_flat_map.h"Please fix this WARNING reported by autoreview issue finding: This header does not appear to be used in this file and can be removed.
#include <array>Please fix this WARNING reported by autoreview issue finding: These headers (`<array>`, `<limits>`) do not appear to be used in this file and can be removed.
static_assert(IsLanguageSubtag("en"));add tests for empty strings to all of these?
// Constexpr check for full tag parsing from a span."string"? or did you mean to use a span below?
const std::optional<ParsedBcp47Tag> parsed = ParseBcp47Tag("en-US");this is the string variant -- do you mean to call the span variant here?
ASSERT_TRUE(parsed.has_value());replace this with:
```
ASSERT_OK_AND_ASSIGN(auto parsed, ParseBcp47Tag("en-US"));
```
and then use `parsed.` rather than `parsed->` below
comment applies to the tests below, too
Related details
- Danilo Tedeschi
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. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi added 12 comments![]( "Open in Gerrit")
constexpr std::optional<ParsedBcp47Tag> ParseBcp47Tag(std::string_view tag) {Danilo Tedeschi`std::string_view tag LIFETIME_BOUND`
Done
`base::span<const std::string_view> subtags LIFETIME_BOUND`
(#include "base/compiler_specific.h")
The bound is actually in the `tag` string_view that generated the span of subtags. If I add a LIFETIME_BOUND here, the compiler gives me an error if I call this from the ParseBcp47Tag(std::string_view tag) overload.
Please fix this WARNING reported by autoreview issue finding: There are a few typos in these comments:
- `IsLanguageSubgtag` -> `IsLanguageSubtag`
- `IsScriptSubgtag` -> `IsScriptSubtag`
- `IsRegionSubgtag` -> `IsRegion` (the function is named `IsRegion`)
- `IsVariantSubgtag` -> `IsVariant` (the function is named `IsVariant`)
fixed and renamed IsRegion, IsVariant -> IsRegionSubtag, IsVariantSubtag.
// The parsed BCP47 tag. It is a view on the actual input string, be-aware of
// life-time.i find this wording a little odd. consider removing this since we can use an annotation to enforce the correct behavior; see below.
Done
return std::ranges::all_of(Danilo Tedeschi#include <algorithm>
Done
i recently became aware of the guidance to avoid generic nested namespace named like `internal` in go/totw/130#best-practices. wdyt about `base::i18n_internal` here? can migrate other files in a distinct CL.
Done
Please fix this WARNING reported by autoreview issue finding: This header does not appear to be used in this file and can be removed.
Done
Please fix this WARNING reported by autoreview issue finding: These headers (`<array>`, `<limits>`) do not appear to be used in this file and can be removed.
Done
add tests for empty strings to all of these?
Done
"string"? or did you mean to use a span below?
Done
const std::optional<ParsedBcp47Tag> parsed = ParseBcp47Tag("en-US");this is the string variant -- do you mean to call the span variant here?
Done
replace this with:
```
ASSERT_OK_AND_ASSIGN(auto parsed, ParseBcp47Tag("en-US"));
```
and then use `parsed.` rather than `parsed->` belowcomment applies to the tests below, too
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Greg Thompson
| Code-Review | +1 |
1 comment
ASSERT_OK_AND_ASSIGN(ParsedBcp47Tag parsed,nit: could use just `parsed` here and get rid of the extra scopes
Related details
- Danilo Tedeschi
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. |