[PATCH] D46699: [ThinLTO] Print module summary index to assembly

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 25 15:42:33 PDT 2018


tejohnson added inline comments.


================
Comment at: lib/AsmParser/LLParser.cpp:741
+    case lltok::Eof:
+      return TokError("found end of file while parsing summary entry");
+    default:
----------------
pcc wrote:
> Can you write tests for the parse failures?
will do


================
Comment at: lib/AsmParser/LLParser.cpp:748
+  } while (NumOpenParen > 0);
+  if (!FoundSomeParens)
+    return TokError(
----------------
pcc wrote:
> This code is reachable only if the first token after the "label" is not an lparen, no? There also seems to be a bug: this code will accept `foo: ) (`. Maybe a more straightforward way to write this is:
> - expect the lparen
> - do the same parsing loop as before, but with NumOpenParen initialized to 1 and no FoundSomeParens variable
> This code is reachable only if the first token after the "label" is not an lparen, no?

That's what was intended, since we will error for any other imbalanced parentheses (by hitting Eof).

> There also seems to be a bug: this code will accept foo: ) (

True, because NumOpenParens is unsigned, and the rparens first will cause it to wrap around and be >0. If I change NumOpenParens to be a signed integer, the loop will exit after the ")" is parsed and we will get the error here here because FoundSomeParens will be false.

But in any case, I will restructure as you suggest, because the current code won't catch the following case:

label: foobarjunk ( )

i.e. it won't complain about foobarjunk before the "(". I don't really want this code to do a lot of syntax checking, because that's coming in the parsing patch, but it sounds reasonable to expect an initial lparen and start at 1. Will change.


Repository:
  rL LLVM

https://reviews.llvm.org/D46699





More information about the llvm-commits mailing list