rsmith added subscribers: echristo, dblaikie, rjmccall.
rsmith 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">;
----------------
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.
================
Comment at: lib/AST/ASTContext.cpp:6261
+ case BuiltinType::ULongAccum:
+ llvm_unreachable("No ObjC encoding for fixed point types");
+
----------------
`llvm_unreachable` should only be used for cases where you believe the
situation to be impossible. I would add these cases to the `return ' ';` case
above (for `Float16` / `Float128` / `Half`) for now.
================
Comment at: lib/AST/ExprConstant.cpp:7323-7325
+ case BuiltinType::UShortAccum:
+ case BuiltinType::UAccum:
+ case BuiltinType::ULongAccum:
----------------
Have you checked what value GCC uses for these? Using `integer_type_class` for
a non-integer value seems very strange to me, but then they probably are
*passed* as integers, so maybe it's correct?
================
Comment at: lib/AST/ItaniumMangle.cpp:2552
+ case BuiltinType::ULongAccum:
+ llvm_unreachable("Fixed point types are disabled for c++");
case BuiltinType::Half:
----------------
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.
================
Comment at: lib/CodeGen/CGDebugInfo.cpp:678-681
+ case BuiltinType::UShortAccum:
+ case BuiltinType::UAccum:
+ case BuiltinType::ULongAccum:
Encoding = llvm::dwarf::DW_ATE_unsigned;
----------------
@echristo @dblaikie Is this appropriate?
================
Comment at: lib/CodeGen/CodeGenTypes.cpp:448
+ case BuiltinType::ULongAccum:
ResultType = llvm::IntegerType::get(getLLVMContext(),
static_cast<unsigned>(Context.getTypeSize(T)));
----------------
jakehehrlich wrote:
> Add TODO for accum types using a different drawf type.
I think this TODO was added to the wrong file. Should it be in CGDebugInfo?
================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2684
// Types added here must also be added to EmitFundamentalRTTIDescriptors.
switch (Ty->getKind()) {
----------------
Note this comment :)
================
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:
> 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?)
================
Comment at: lib/Sema/SemaType.cpp:1390-1394
+ if (S.getLangOpts().CPlusPlus) {
+ S.Diag(DS.getTypeSpecTypeLoc(), diag::err_fixed_point_only_allowed_in_c);
+ Result = Context.ShortAccumTy;
+ break;
+ }
----------------
As noted earlier, we should not have such a restriction.
================
Comment at: lib/Sema/SemaType.cpp:1424
+ case DeclSpec::TSW_longlong:
+ // TODO: Replace with diag
+ llvm_unreachable("Unable to specify long long as _Accum width");
----------------
No need for a diagnostic as this is already caught elsewhere.
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
