LGTM. -Eli
On Mon, Sep 9, 2013 at 5:04 AM, Stepan Dyatkovskiy <[email protected]>wrote: > Since I had split the patch and committed only ASTContext and TargetInfo > getIntTypeByWidth and friends, there is the second patch, that fixes > PR16752 itself. > > Please, find it in attachment for review. > > -Stepan. > Eli Friedman wrote: > >> On Tue, Sep 3, 2013 at 2:16 AM, Stepan Dyatkovskiy <[email protected] >> <mailto:[email protected]>> wrote: >> >> Hi Eli, >> Sorry for latency. >> As you remember this patch should correct 'mode' attr >> implementation. You proposed to use generic way of type detection >> for each target: just scan for target types and select one with >> suitable width. >> >> Unfortunately I can't commit patch with generic implementation. I >> got test failure for clang. And I expect more failures for another >> targets. The reason is next. >> >> The target could still keep mode unsupported even if it has type of >> suitable width. I mean the next. >> For example this string should cause error for i686 machines (128 >> bit float): >> typedef float f128ibm __attribute__ ((mode (TF))); >> But in case of generic approach for all targets it would be passed. >> In this case virtual functions allows to implement exceptions. >> >> >> The generic implementation is still essentially correct; the the issue >> is that getLongDoubleWidth() isn't actually the right value to check. >> It returns "sizeof(long double) * 8", not the actual width of the >> underlying float format. You should be able to work around this by >> checking getLongDoubleFormat() instead of getLongDoubleWidth(). >> >> In this case 'getIntTypeByWidth' may be a bit confusing name. >> Instead it is supposed to return type for some particular mode, not >> by width. Something like getInt/RealTypeForMode(width, sign). >> Though, currently I kept the original name. >> >> >> If you want to change it, that's fine. >> >> -Eli >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
