[PATCH] D121067: [YAMLParser] Refactor BlockStyle and BlockChomping type
Scott Linder via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 8 10:26:54 PST 2022
scott.linder added a comment.
Thank you for following up! The change looks correct, but I have some nits about the general approach. Let me know what you think.
================
Comment at: llvm/lib/Support/YAMLParser.cpp:163
+enum BlockStyleIndicator : char {
+ BSI_None = ' ',
+ BSI_Literal = '|',
----------------
This isn't actually a possible style, right? It seems like we could instead represent the cases where this comes up by using an `Optional<BlockStyleIndicator>`, which would keep this enum 1:1 with the YAML spec.
================
Comment at: llvm/lib/Support/YAMLParser.cpp:169
+enum BlockChompingIndicator : char {
+ BCI_Clip = ' ',
+ BCI_Strip = '-',
----------------
I think this representation is subtley different than the spec, where `CLIP` is just the default and applies whenever the indicator is "empty" or absent. This small inconsistency would also push me towards using the `enum class` approach.
================
Comment at: llvm/lib/Support/YAMLParser.cpp:1566-1568
+ if (Current != End) {
+ if (*Current == '>') {
+ Indicator = BSI_Folded;
----------------
This feels like this straddles multiple possible implementations, with a mix of the benefits/problems of each.
I would suggest using a type-safe `enum class` for the new enumerations; in that case, the `char` comparisons here are the single source-of-truth, at which point we enter a type safe world where one cannot accidentally compare the enumeration values to `char`. It also has the (subjectively) nice property that it is properly namespaced, so the `BSI_` and `BCI_` prefixes aren't necessary. While at it, I'd address the removal of `None` as a possibility, and hide some of the details by using `Scanner::consume`. All together, I'd suggest something like:
```
enum class BlockStyleIndicator {
Literal,
Folded
};
Optional<BlockStyleIndicator> Scanner::scanBlockStyleIndicator() {
if (consume('>'))
return BlockStyleIndicator::Folded;
if (consume('|'))
return BlockStyleIndicator::Literal;
return None;
}
```
Alternatively, with the version you have using non-`class` `enum`s the `'>'` here could be `BSI_Folded`, or you could just leave the implementation as-is, using the implicit conversion from `char` to the enum type. I think this approach is perfectly valid, but it isn't the first one I reach for if there is no other compelling reason.
================
Comment at: llvm/lib/Support/YAMLParser.cpp:1578-1590
+BlockChompingIndicator Scanner::scanBlockChompingIndicator() {
+ BlockChompingIndicator Indicator = BCI_Clip;
+ if (Current != End) {
+ if (*Current == '+') {
+ Indicator = BCI_Keep;
+ skip(1);
+ } else if (*Current == '-') {
----------------
This would get a similar treatment as above, assuming we go the `enum class` route.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121067/new/
https://reviews.llvm.org/D121067
More information about the llvm-commits
mailing list