[PATCH] D50930: Move Itanium demangler implementation into a header file and add visitation support.

[Erik Pilkington via Phabricator via llvm-commits]

erik.pilkington added inline comments.


================
Comment at: llvm/trunk/include/llvm/Demangle/ItaniumDemangle.h:609
+      assert(FirstChar == SecondChar);
+      ++FirstChar, ++SecondChar;
+    }
----------------
xbolva00 wrote:
> erik.pilkington wrote:
> > xbolva00 wrote:
> > > Hi Richard, this code looks suspicious. Is this correct?
> > > 
> > > PVS warns:
> > > /home/xbolva00/LLVM/llvm-project/llvm/include/llvm/Demangle/ItaniumDemangle.h	620	err	V769 The 'SecondChar' pointer in the '++ SecondChar' expression equals nullptr. The resulting value is senseless and it should not be used.
> > > 
> > > 
> > (This is my fault, Richard just copied this)
> > 
> > This is sorta fine, this done to distinguish between an empty StringView with nullptr first and last and an empty NodeOrString. The pointers are never loaded from, but I believe this is still UB. That being said, I think this class is over engineered and we'd probably be better off putting the StringView in a `NameType` in cases where we need to have either a Node or a String. 
> Thanks for info. Can you add code comment here for now?
> 
>  
I'll just eradicate it when I'm back at my desk on Monday, shouldn't be too tricky. IIRC this is only used in a few places.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D50930