[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: lib/Index/USRGeneration.cpp:691
+        case BuiltinType::ULongAccum:
+          llvm_unreachable("No USR name mangling for fixed point types.");
         case BuiltinType::Float16:
----------------
leonardchan wrote:
> rsmith wrote:
> > leonardchan wrote:
> > > phosek wrote:
> > > > We need some solution for fixed point types.
> > > Added character ~ to indicate fixed point type followed by string detailing the type. I have not added a test to it because logically, I do not think we will ever reach that point. This logic is implemented in the VisitType method, which mostly gets called by visitors to c++ nodes like VisitTemplateParameterList, but we have disabled the use of fixed point types in c++. VisitType does get called in VisitFunctionDecl but the function exits early since we are not reading c++ (line lib/Index/USRGeneration.cpp:238).
> > @rjmccall Is this an acceptable USR encoding? (Is our USR encoding scheme documented anywhere?)
> I chatted with @sammccall about this who said it was ok to add these types if no one opposed this. I posted this on cfe-dev also and it seemed that no one spoke up about it, so I thought this was ok.
> 
> I also couldn't find any standard or documentation about reserving characters for USR. It doesn't seem that USR is also parsed in any way, so I don't think I'm breaking anything (running ninja check-all passes).
> 
> And unless we also do enable this for C++, this code actually may not be run since this method will not be visited if limited to C.
If the conclusion is to only allow these types for C, then you could punt on this by setting IgnoreResult so USR generation fails. As you say USRs only include type information in C++.

I think nothing parses them except humans, and using a prefix character is probably better than eating 24 single characters. But one prefix character + one trailing character might be less surprising.


Repository:
  rC Clang

https://reviews.llvm.org/D46084