[YAMLParser] Refactor BlockStyle and BlockChomping type

[HassanElDesouky]

Summary

In D102590, we agreed to create a BlockStyle and BlockChomping enums. This Diff address that change.

[HassanElDesouky]

@scott.linder Would you mind giving this a review?
Please, if we are going to merge this can you author the commit for me? Here's my GitHub email [e-mail]

[scott.linder]

Thank you for following up! The change looks correct, but I have some nits about the general approach. Let me know what you think.

llvm/lib/Support/YAMLParser.cpp

163

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.

169

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.

1566–1568

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 enums 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.

1578–1590

This would get a similar treatment as above, assuming we go the enum class route.

llvm/lib/Support/YAMLParser.cpp

   Token() = default;
 };
 
+enum BlockStyleIndicator : char {
+  BSI_None = ' ',
+  BSI_Literal = '|',
+  BSI_Folded = '>'
+};
+
+enum BlockChompingIndicator : char {
+  BCI_Clip = ' ',
+  BCI_Strip = '-',
+  BCI_Keep = '+'
+};
+
 } // end namespace yaml
 } // end namespace llvm
 

   /// YAML does not consider the style indicator to be a part of the header.
   ///
   /// Return false if an error occurred.
-  bool scanBlockScalarIndicators(char &StyleIndicator, char &ChompingIndicator,
+  bool scanBlockScalarIndicators(BlockStyleIndicator &StyleIndicator,
+                                 BlockChompingIndicator &ChompingIndicator,
                                  unsigned &IndentIndicator, bool &IsDone);
 
   /// Scan a style indicator in a block scalar header.
-  char scanBlockStyleIndicator();
+  BlockStyleIndicator scanBlockStyleIndicator();
 
   /// Scan a chomping indicator in a block scalar header.
-  char scanBlockChompingIndicator();
+  BlockChompingIndicator scanBlockChompingIndicator();
 
   /// Scan an indentation indicator in a block scalar header.
   unsigned scanBlockIndentationIndicator();

   /// Scan a block scalar header.
   ///
   /// Return false if an error occurred.
-  bool scanBlockScalarHeader(char &ChompingIndicator, unsigned &IndentIndicator,
-                             bool &IsDone);
+  bool scanBlockScalarHeader(BlockChompingIndicator &ChompingIndicator,
+                             unsigned &IndentIndicator, bool &IsDone);
 
   /// Look for the indentation level of a block scalar.
   ///

   return true;
 }
 
-bool Scanner::scanBlockScalarIndicators(char &StyleIndicator,
-                                        char &ChompingIndicator,
-                                        unsigned &IndentIndicator,
-                                        bool &IsDone) {
+bool Scanner::scanBlockScalarIndicators(
+    BlockStyleIndicator &StyleIndicator,
+    BlockChompingIndicator &ChompingIndicator, unsigned &IndentIndicator,
   StyleIndicator = scanBlockStyleIndicator();
   if (!scanBlockScalarHeader(ChompingIndicator, IndentIndicator, IsDone))
     return false;
   return true;
 }
 
-char Scanner::scanBlockStyleIndicator() {
-  char Indicator = ' ';
-  if (Current != End && (*Current == '>' || *Current == '|')) {
-    Indicator = *Current;
-    skip(1);
+BlockStyleIndicator Scanner::scanBlockStyleIndicator() {
+  BlockStyleIndicator Indicator = BSI_None;
+  if (Current != End) {
+    if (*Current == '>') {
+      Indicator = BSI_Folded;
+      skip(1);
+    } else if (*Current == '|') {
+      Indicator = BSI_Literal;
+      skip(1);
+    }
   }
   return Indicator;
 }
 
-char Scanner::scanBlockChompingIndicator() {
-  char Indicator = ' ';
-  if (Current != End && (*Current == '+' || *Current == '-')) {
-    Indicator = *Current;
-    skip(1);
+BlockChompingIndicator Scanner::scanBlockChompingIndicator() {
+  BlockChompingIndicator Indicator = BCI_Clip;
+  if (Current != End) {
+    if (*Current == '+') {
+      Indicator = BCI_Keep;
+      skip(1);
+    } else if (*Current == '-') {
+      Indicator = BCI_Strip;
+      skip(1);
+    }
   }
   return Indicator;
 }

 ///
 /// Return the number of trailing line breaks to emit, depending on
 /// \p ChompingIndicator.
-static unsigned getChompedLineBreaks(char ChompingIndicator,
+static unsigned getChompedLineBreaks(BlockChompingIndicator ChompingIndicator,
                                      unsigned LineBreaks, StringRef Str) {
-  if (ChompingIndicator == '-') // Strip all line breaks.
+  if (ChompingIndicator == BCI_Strip) // Strip all line breaks.
     return 0;
-  if (ChompingIndicator == '+') // Keep all line breaks.
+  if (ChompingIndicator == BCI_Keep) // Keep all line breaks.
     return LineBreaks;
   // Clip trailing lines.
   return Str.empty() ? 0 : 1;

   return Indent;
 }
 
-bool Scanner::scanBlockScalarHeader(char &ChompingIndicator,
+bool Scanner::scanBlockScalarHeader(BlockChompingIndicator &ChompingIndicator,
                                     unsigned &IndentIndicator, bool &IsDone) {
   auto Start = Current;
 
   ChompingIndicator = scanBlockChompingIndicator();
   IndentIndicator = scanBlockIndentationIndicator();
   // Check for the chomping indicator once again.
-  if (ChompingIndicator == ' ')
+  if (ChompingIndicator == BCI_Clip)
     ChompingIndicator = scanBlockChompingIndicator();
   Current = skip_while(&Scanner::skip_s_white, Current);
   skipComment();

 
 bool Scanner::scanBlockScalar(bool IsLiteral) {
   assert(*Current == '|' || *Current == '>');
-  char StyleIndicator;
-  char ChompingIndicator;
+  BlockStyleIndicator StyleIndicator;
+  BlockChompingIndicator ChompingIndicator;
   unsigned BlockIndent;
   bool IsDone = false;
   if (!scanBlockScalarIndicators(StyleIndicator, ChompingIndicator, BlockIndent,

     return false;
   if (IsDone)
     return true;
-  bool IsFolded = StyleIndicator == '>';
 
   const auto *Start = Current;
   unsigned BlockExitIndent = Indent < 0 ? 0 : (unsigned)Indent;

     auto LineStart = Current;
     advanceWhile(&Scanner::skip_nb_char);
     if (LineStart != Current) {
-      if (LineBreaks && IsFolded && !Scanner::isLineEmpty(Str)) {
+      if (LineBreaks && StyleIndicator == BSI_Folded &&
+          !Scanner::isLineEmpty(Str)) {
         // The folded style "folds" any single line break between content into a
         // single space, except when that content is "empty" (only contains
         // whitespace) in which case the line break is left as-is.