Merge pull request #478 from biojppm/fix/475_trailing_space

biojppm · Jan 19, 2025 · 83595d0 · 83595d0
2 parents 75e1034 + 9d70e69
commit 83595d0
Show file tree

Hide file tree

Showing 4 changed files with 201 additions and 43 deletions.
diff --git a/changelog/current.md b/changelog/current.md
@@ -1,6 +1,7 @@
 ## Fixes
 
 - [PR#488](https://github.com/biojppm/rapidyaml/pull/488):
-  - add workarounds for problems with codegen of gcc 11,12,13
-  - improve CI coverage of gcc and clang optimization levels
+  - add workarounds for problems with codegen of gcc 11,12,13.
+  - improve CI coverage of gcc and clang optimization levels.
 - [BREAKING] Fix [#477](https://github.com/biojppm/rapidyaml/issues/477): changed `read<std::map>()` to overwrite existing entries. The provided implementations had an inconsistency between `std::map` (which wasn't overwriting) and `std::vector` (which *was* overwriting).
+- Fix [#475](https://github.com/biojppm/rapidyaml/issues/475): parse error on trailing whitespace in block containers.
diff --git a/src/c4/yml/parse_engine.def.hpp b/src/c4/yml/parse_engine.def.hpp
@@ -4064,17 +4064,16 @@ bool ParseEngine<EventHandler>::_locations_dirty() const
 template<class EventHandler>
 void ParseEngine<EventHandler>::_handle_flow_skip_whitespace()
 {
+    // don't assign to csubstr rem: otherwise, gcc12,13,14 -O3 -m32 misbuilds
     if(m_evt_handler->m_curr->line_contents.rem.len > 0)
     {
-        csubstr rem = m_evt_handler->m_curr->line_contents.rem;
-        if(rem.str[0] == ' ' || rem.str[0] == '\t')
+        if(m_evt_handler->m_curr->line_contents.rem.str[0] == ' ' || m_evt_handler->m_curr->line_contents.rem.str[0] == '\t')
         {
-            _c4dbgpf("starts with whitespace: '{}'", _c4prc(rem.str[0]));
+            _c4dbgpf("starts with whitespace: '{}'", _c4prc(m_evt_handler->m_curr->line_contents.rem.str[0]));
             _skipchars(" \t");
-            rem = m_evt_handler->m_curr->line_contents.rem;
         }
         // comments
-        if(rem.begins_with('#'))
+        if(m_evt_handler->m_curr->line_contents.rem.begins_with('#'))
         {
             _c4dbgpf("it's a comment: {}", m_evt_handler->m_curr->line_contents.rem);
             _line_progressed(m_evt_handler->m_curr->line_contents.rem.len);
@@ -4925,14 +4924,14 @@ void ParseEngine<EventHandler>::_handle_seq_flow()
     _RYML_CB_ASSERT(m_evt_handler->m_stack.m_callbacks, m_evt_handler->m_curr->indref != npos);
 
     _handle_flow_skip_whitespace();
-    csubstr rem = m_evt_handler->m_curr->line_contents.rem;
-    if(!rem.len)
+    // don't assign to csubstr rem: otherwise, gcc12,13,14 -O3 -m32 misbuilds
+    if(!m_evt_handler->m_curr->line_contents.rem.len)
         goto seqflow_again;
 
     if(has_any(RVAL))
     {
         _RYML_CB_ASSERT(m_evt_handler->m_stack.m_callbacks, has_none(RNXT));
-        const char first = rem.str[0];
+        const char first = m_evt_handler->m_curr->line_contents.rem.str[0];
         ScannedScalar sc;
         if(first == '\'')
         {
@@ -5048,7 +5047,7 @@ void ParseEngine<EventHandler>::_handle_seq_flow()
     {
         _RYML_CB_ASSERT(m_evt_handler->m_stack.m_callbacks, has_any(RNXT));
         _RYML_CB_ASSERT(m_evt_handler->m_stack.m_callbacks, has_none(RVAL));
-        const char first = rem.str[0];
+        const char first = m_evt_handler->m_curr->line_contents.rem.str[0];
         if(first == ',')
         {
             _c4dbgp("seqflow[RNXT]: expect next val");
@@ -5813,43 +5812,63 @@ void ParseEngine<EventHandler>::_handle_seq_block()
         // handle indentation
         //
         _c4dbgpf("seqblck[RNXT]: indref={} indentation={}", m_evt_handler->m_curr->indref, m_evt_handler->m_curr->line_contents.indentation);
-        if(C4_UNLIKELY(!_at_line_begin()))
-            _c4err("parse error");
-        if(m_evt_handler->m_curr->indentation_ge())
-        {
-            _c4dbgpf("seqblck[RNXT]: skip {} from indref", m_evt_handler->m_curr->indref);
-            _line_progressed(m_evt_handler->m_curr->indref);
-            _maybe_skip_whitespace_tokens();
-            rem = m_evt_handler->m_curr->line_contents.rem;
-            if(!rem.len)
-                goto seqblck_again;
-        }
-        else if(m_evt_handler->m_curr->indentation_lt())
+        if(C4_LIKELY(_at_line_begin()))
         {
-            _c4dbgp("seqblck[RNXT]: smaller indentation!");
-            _handle_indentation_pop_from_block_seq();
-            if(has_all(RSEQ|BLCK))
+            _c4dbgp("seqblck[RNXT]: at line begin");
+            if(m_evt_handler->m_curr->indentation_ge())
             {
-                _c4dbgp("seqblck[RNXT]: still seqblck!");
-                _RYML_CB_ASSERT(m_evt_handler->m_stack.m_callbacks, has_any(RNXT));
-                _line_progressed(m_evt_handler->m_curr->line_contents.indentation);
+                _c4dbgpf("seqblck[RNXT]: skip {} from indref", m_evt_handler->m_curr->indref);
+                _line_progressed(m_evt_handler->m_curr->indref);
+                _maybe_skip_whitespace_tokens();
                 rem = m_evt_handler->m_curr->line_contents.rem;
                 if(!rem.len)
                     goto seqblck_again;
             }
-            else
+            else if(m_evt_handler->m_curr->indentation_lt())
             {
-                _c4dbgp("seqblck[RNXT]: no longer seqblck!");
-                goto seqblck_finish;
+                _c4dbgp("seqblck[RNXT]: smaller indentation!");
+                _handle_indentation_pop_from_block_seq();
+                if(has_all(RSEQ|BLCK))
+                {
+                    _c4dbgp("seqblck[RNXT]: still seqblck!");
+                    _RYML_CB_ASSERT(m_evt_handler->m_stack.m_callbacks, has_any(RNXT));
+                    _line_progressed(m_evt_handler->m_curr->line_contents.indentation);
+                    rem = m_evt_handler->m_curr->line_contents.rem;
+                    if(!rem.len)
+                        goto seqblck_again;
+                }
+                else
+                {
+                    _c4dbgp("seqblck[RNXT]: no longer seqblck!");
+                    goto seqblck_finish;
+                }
+            }
+            else if(m_evt_handler->m_curr->line_contents.indentation == npos)
+            {
+                _c4dbgpf("seqblck[RNXT]: blank line, len={}", m_evt_handler->m_curr->line_contents.rem);
+                _line_progressed(m_evt_handler->m_curr->line_contents.rem.len);
+                rem = m_evt_handler->m_curr->line_contents.rem;
+                if(!rem.len)
+                    goto seqblck_again;
             }
         }
-        else if(m_evt_handler->m_curr->line_contents.indentation == npos)
+        else
         {
-            _c4dbgpf("seqblck[RNXT]: blank line, len={}", m_evt_handler->m_curr->line_contents.rem);
-            _line_progressed(m_evt_handler->m_curr->line_contents.rem.len);
-            rem = m_evt_handler->m_curr->line_contents.rem;
-            if(!rem.len)
-                goto seqblck_again;
+            _c4dbgp("seqblck[RNXT]: NOT at line begin");
+            if(!rem.begins_with_any(" \t"))
+            {
+                _c4err("parse error");
+            }
+            else
+            {
+                _skipchars(" \t");
+                rem = m_evt_handler->m_curr->line_contents.rem;
+                if(!rem.len)
+                {
+                    _c4dbgp("seqblck[RNXT]: again");
+                    goto seqblck_again;
+                }
+            }
         }
         //
         // now handle the tokens
@@ -6750,6 +6769,24 @@ void ParseEngine<EventHandler>::_handle_map_block()
                 }
             }
         }
+        else
+        {
+            _c4dbgp("mapblck[RNXT]: NOT at line begin");
+            if(!rem.begins_with_any(" \t"))
+            {
+                _c4err("parse error");
+            }
+            else
+            {
+                _skipchars(" \t");
+                rem = m_evt_handler->m_curr->line_contents.rem;
+                if(!rem.len)
+                {
+                    _c4dbgp("seqblck[RNXT]: again");
+                    goto mapblck_again;
+                }
+            }
+        }
         //
         // handle tokens
         //

diff --git a/test/test_github_issues.cpp b/test/test_github_issues.cpp
@@ -4,6 +4,117 @@
 namespace c4 {
 namespace yml {
 
+TEST(github, 475_0_space)
+{
+    Tree t;
+    ExpectError::check_success(&t, [&t]{
+        parse_in_arena(R"(
+test:
+- {a: 1}
+# next line has a trailing space
+- {b: 2} 
+# next line has a trailing space
+- [0, {c: 3}] 
+)", &t);
+    });
+    ConstNodeRef test = t["test"];
+    ASSERT_TRUE(test.is_seq());
+    ASSERT_EQ(test.num_children(), 3u);
+}
+
+TEST(github, 475_1_space_indented)
+{
+    Tree t;
+    ExpectError::check_success(&t, [&t]{
+        parse_in_arena(R"(
+test:
+  - {a: 1}
+  # next line has a trailing space
+  - {b: 2} 
+  # next line has a trailing space
+  - [0, {c: 3}] 
+)", &t);
+    });
+    ConstNodeRef test = t["test"];
+    ASSERT_TRUE(test.is_seq());
+    ASSERT_EQ(test.num_children(), 3u);
+}
+
+TEST(github, 475_2_tab)
+{
+    Tree t;
+    ExpectError::check_success(&t, [&t]{
+        parse_in_arena(R"(
+test:
+- {a: 1}
+# next line has a trailing tab
+- {b: 2}	
+# next line has a trailing tab
+- [0, {c: 3}]	
+)", &t);
+    });
+    ConstNodeRef test = t["test"];
+    ASSERT_TRUE(test.is_seq());
+    ASSERT_EQ(test.num_children(), 3u);
+}
+
+TEST(github, 475_3_tab_indented)
+{
+    Tree t;
+    ExpectError::check_success(&t, [&t]{
+        parse_in_arena(R"(
+test:
+  - {a: 1}
+  # next line has a trailing tab
+  - {b: 2}	
+  # next line has a trailing tab
+  - [0, {c: 3}]	
+)", &t);
+    });
+    ConstNodeRef test = t["test"];
+    ASSERT_TRUE(test.is_seq());
+    ASSERT_EQ(test.num_children(), 3u);
+}
+
+TEST(github, 475_4_space_map)
+{
+    Tree t;
+    ExpectError::check_success(&t, [&t]{
+        parse_in_arena(R"(
+test:
+  0: {a: 1}
+  # next line has a trailing space
+  1: {b: 2} 
+  # next line has a trailing space
+  2: [0, {c: 3}] 
+)", &t);
+    });
+    ConstNodeRef test = t["test"];
+    ASSERT_TRUE(test.is_map());
+    ASSERT_EQ(test.num_children(), 3u);
+}
+
+TEST(github, 475_5_tab_map)
+{
+    Tree t;
+    ExpectError::check_success(&t, [&t]{
+        parse_in_arena(R"(
+test:
+  0: {a: 1}
+  # next line has a trailing tab
+  1: {b: 2}	
+  # next line has a trailing tab
+  2: [0, {c: 3}]	
+)", &t);
+    });
+    ConstNodeRef test = t["test"];
+    ASSERT_TRUE(test.is_map());
+    ASSERT_EQ(test.num_children(), 3u);
+}
+
+
+//-----------------------------------------------------------------------------
+
 TEST(github, 455_0_ok)
 {
     Tree t;

diff --git a/test/test_lib/test_case.cpp b/test/test_lib/test_case.cpp
@@ -237,32 +237,41 @@ ExpectError::ExpectError(Tree *tree, Location loc)
     c4::yml::Callbacks tcb((void*)this, tree ? m_tree_prev.m_allocate : nullptr, tree ? m_tree_prev.m_free : nullptr, err);
     c4::yml::Callbacks gcb((void*)this, m_glob_prev.m_allocate, m_glob_prev.m_free, err);
     #endif
-    _c4dbgp("setting error callback");
     if(tree)
+    {
+        _c4dbgp("setting error callback: tree");
         tree->callbacks(tcb);
+    }
+    _c4dbgp("setting error callback: global");
     set_callbacks(gcb);
 }
 
 ExpectError::~ExpectError()
 {
     if(m_tree)
+    {
+        _c4dbgp("resetting error callback: tree");
         m_tree->callbacks(m_tree_prev);
+    }
+    _c4dbgp("resetting error callback: global");
     set_callbacks(m_tree_prev);
-    _c4dbgp("resetting error callback");
 }
 
 void ExpectError::check_success(Tree *tree, std::function<void()> fn)
 {
-    auto context = ExpectError(tree, {});
+    Location expected_location = {};
+    auto context = ExpectError(tree, expected_location);
     C4_IF_EXCEPTIONS_(try, if(setjmp(s_jmp_env_expect_error) == 0))
     {
+        _c4dbgp("check expected success");
         fn();
+        _c4dbgp("check expected success: success!");
     }
     C4_IF_EXCEPTIONS_(catch(ExpectedError const&), else)
     {
-        ;
+        FAIL() << "check expected success: failed!";
     }
-    EXPECT_FALSE(context.m_got_an_error);
+    ASSERT_FALSE(context.m_got_an_error);
 }
 
 void ExpectError::check_error(Tree const* tree, std::function<void()> fn, Location expected_location)