[PATCH] D117879: [demangler][NFC] Refactor some parsing

[Nathan Sidwell via Phabricator via llvm-commits]

urnathan added inline comments.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:2591-2594
+  if (look() == 'N') {
+    Result = getDerived().parseNestedName(State);
+  } else if (look() == 'Z') {
+    Result = getDerived().parseLocalName(State);
----------------
aaron.ballman wrote:
> Personally, I think the early returns are a bit easier to follow along with. WDYT?
I can ramble on a bit here.  TL;DR: not particularly wedded to this particular change.  This variant has a helpful blank line after the 'if () return ...' sequence.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:4077
   case 'S': {
-    if (look(1) && look(1) != 't') {
-      Node *Sub = getDerived().parseSubstitution();
-      if (Sub == nullptr)
+    if (look(1) != 't') {
+      Result = getDerived().parseSubstitution();
----------------
aaron.ballman wrote:
> Was this change NFC, or did we drop a needed check for `\0` because there is nothing to lookahead to?
This is NFC.  The previous 'look ()' test would cause us not to enter this block, but then fail later as 'S\0'[*]  has no encoding.  Removing that test causes us to enter this block and barf sooner.

look returns NUL on both end-of-string and of course an actual NUL.  They're both equally ill-formed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117879/new/

https://reviews.llvm.org/D117879