leonardchan added a subscriber: sammccall.
leonardchan added inline comments.
================
Comment at: include/clang/Basic/DiagnosticCommonKinds.td:172
+def err_fixed_point_only_allowed_in_c : Error<
+ "Fixed point types are only allowed in C">;
----------------
rsmith wrote:
> Diagnostics should not be capitalized. Also, we generally allow conforming C
> extensions to be used in other languages unless there is a really good reason
> not to.
We decided not to allow fixed point types in other languages because there is
no specification provided in N1169 for addressing some features in other
languages. Using C++ as an example, N1169 does not provide recommended
characters when name mangling so we do not allow this in C++.
================
Comment at: lib/AST/ItaniumMangle.cpp:2552
+ case BuiltinType::ULongAccum:
+ llvm_unreachable("Fixed point types are disabled for c++");
case BuiltinType::Half:
----------------
rsmith wrote:
> Please check what GCC uses to mangle these, and follow suit; if GCC doesn't
> have a mangling, you can use a vendor mangling (`u6_Accum`) or produce an
> error for now, but please open an issue at
> https://github.com/itanium-cxx-abi/cxx-abi/ to pick a real mangling.
It seems that GCC uses the characters for each fixed point type's corresponding
integral type (https://github.com/gcc-mirror/gcc/blob/master/gcc/cp/mangle.c).
Will follow up on this if we end up enabling fixed point types for C++.
================
Comment at: lib/CodeGen/CGDebugInfo.cpp:678-681
+ case BuiltinType::UShortAccum:
+ case BuiltinType::UAccum:
+ case BuiltinType::ULongAccum:
Encoding = llvm::dwarf::DW_ATE_unsigned;
----------------
rsmith wrote:
> @echristo @dblaikie Is this appropriate?
My bad, this should be changed to `DW_ATE_signed_fixed` and
`DW_ATE_unsigned_fixed`
================
Comment at: lib/Index/USRGeneration.cpp:691
+ case BuiltinType::ULongAccum:
+ llvm_unreachable("No USR name mangling for fixed point types.");
case BuiltinType::Float16:
----------------
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.
Repository:
rC Clang
https://reviews.llvm.org/D46084
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
