[llvm] [YAMLParser] Improve plain scalar spec compliance (PR #68946)

via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 16 19:33:30 PDT 2023


================
@@ -3153,7 +3153,7 @@ TEST(YAMLIO, TestFlowSequenceTokenErrors) {
 
 TEST(YAMLIO, TestDirectiveMappingNoValue) {
   Input yin("%YAML\n{5:");
-  EXPECT_FALSE(yin.setCurrentDocument());
+  yin.setCurrentDocument();
----------------
akirchhoff-modular wrote:

Yes, it does change with this PR. I spent a long time investigating why. If I recall:

* Prior to this PR, we failed at some point during directive parsing, causing the document to never be set.
* After this PR, we still fail, but it is at a later point while parsing the map, after the document has already been set.

I had looked at the blame and seen that this test was added when fixing some bugs found by fuzz testing (in particular, I believe this test is there to prevent regressions of a bug in how `KeyValueNode` handled `nullptr` children). My changes to the scanner cause this code path to no longer be hit, but I am not sure how to find another input that would cause the same code path to be executed, since it seemed to rely on a fairly specific failure mode of the scanner.

I figure it is probably useful to keep this test to the extent that it shows that we continue to not crash on this input, but I don't know a way to adapt it to test the exact same thing it had been testing before -- I do not know if that code path is still reachable at all or not.

https://github.com/llvm/llvm-project/pull/68946


More information about the llvm-commits mailing list