[PATCH] D121067: [YAMLParser] Refactor BlockStyle and BlockChomping type

[Scott Linder via Phabricator via llvm-commits]

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