leonardchan added inline comments.
================ Comment at: include/clang/Basic/TokenKinds.def:393 +// ISO/IEC JTC1 SC22 WG14 N1169 Extension +KEYWORD(_Accum , KEYALL) + ---------------- rsmith wrote: > leonardchan wrote: > > jfb wrote: > > > ebevhan wrote: > > > > I believe that having KEYALL will enable the keyword even if > > > > -fno-fixed-point is given. Is this desired? It will mean that `_Accum` > > > > will not be a valid identifier in standard C regardless of the flag. > > > That seems fine: identifiers starting with underscore and a capital > > > letter already aren't valid identifiers in C and C++ because they're > > > reserved for the implementation. > > I think my test for `-fno-fixed-point` already catches this, but I did not > > notice until now the `KEYNOCXX` keyword until now. Using this instead > > allows for not having to check if the language is c++ since `_Accum` is no > > longer treated as a typename. The corresponding test checking fixed points > > in c++ has been updated to reflect this. > Just to make sure we're on the same page: it's OK to disable this feature for > C++ while you're working on it, but it needs to support C++ by the time > you're done. That's the goal. Just disabled in c++ for now with the end goal of getting c++ support. Right now I'm waiting for these types to be added in the next Itanium ABI, which I believe you responded to on Github. Afterwards I'll need to request Microsoft mangling. ================ Comment at: lib/Basic/TargetInfo.cpp:45 + AccumWidth = AccumAlign = 32; + LongAccumWidth = LongAccumAlign = 64; SuitableAlign = 64; ---------------- rsmith wrote: > jfb wrote: > > This seems weird because Targets which don't have these values for the > > non-Accum versions will have .e.g. `sizeof(short) != sizeof(short _Accum)`. > > Is there a point in ever having `_Accum` differ in size, width, and > > alignment from the underlying type? If not, can you set these values after > > the sub-target has specified its preferences? > I'm uncomfortable about opting all targets into this behavior with these > default values; this will result in an ABI break if later a target updates > these to the correct values. A per-target `AccumSupported` flag would help > substantially. But this is OK for the short term while you're still working > on the feature. > > We'll also need the target to inform us of the number of integer and > fractional bits for each such type. The integral and fractional bits will be set in the target and is available in a later patch. ================ Comment at: lib/Basic/TargetInfo.cpp:45 + AccumWidth = AccumAlign = 32; + LongAccumWidth = LongAccumAlign = 64; SuitableAlign = 64; ---------------- ebevhan wrote: > leonardchan wrote: > > rsmith wrote: > > > jfb wrote: > > > > This seems weird because Targets which don't have these values for the > > > > non-Accum versions will have .e.g. `sizeof(short) != sizeof(short > > > > _Accum)`. Is there a point in ever having `_Accum` differ in size, > > > > width, and alignment from the underlying type? If not, can you set > > > > these values after the sub-target has specified its preferences? > > > I'm uncomfortable about opting all targets into this behavior with these > > > default values; this will result in an ABI break if later a target > > > updates these to the correct values. A per-target `AccumSupported` flag > > > would help substantially. But this is OK for the short term while you're > > > still working on the feature. > > > > > > We'll also need the target to inform us of the number of integer and > > > fractional bits for each such type. > > The integral and fractional bits will be set in the target and is available > > in a later patch. > > We'll also need the target to inform us of the number of integer and > > fractional bits for each such type. > > I believe the only one that is needed is for the number of fractional bits; > the number of integer bits is implied by the difference between the type > width and fractional bits. I think I mention this in one of the other patches. > > You're right. I was stuck in the mindset that we would be providing an integral and fractional value. 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
