Jeffrey Walton wrote: > On Tue, Jul 14, 2015 at 6:20 PM, Robert Roessler wrote: >> Jeffrey Walton wrote: >>> >>> ... >>> OK, the additional warnings on Visual Studio likely came because of >>> >>> https://github.com/weidai11/cryptopp/commit/e67480dd9e8dcda57939fc7480e273b8d2e81335. >>> >>> We also cleared some more misc.h warnings at >>> >>> https://github.com/weidai11/cryptopp/commit/828c550389f79b035f4518a79b7138d281f0961c. >>> >>>> believe the actual complaint from VS (2013 for me) is caused by the >>>> various >>>> low-level workhorse "rotate" functions declaring their "shift" param as >>>> an >>>> unsigned char - which is where the actual alleged truncation is taking >>>> place >>>> (or would). >>> >>> >>> Here's the commit/comment on the truncation to char: >>> >>> https://github.com/weidai11/cryptopp/commit/457eaeaf2317faa30c86d6d3fc289f40c58cf177. >>> It added assert(y <= 255), which turned out to be spurious because of >>> assert(y <= sizeof(T)*8). The latter was really assert(y <= 32). >>> >>> But I'm having trouble parsing what the problem is, or what you want me to >>> do. >>> >>> Where do you see a problem, or what do you suggest we do? >> >> >> I obviously left out some details... like what exactly I was talking about. >> ;) >> >> I get the C4242 issues (in the MSVC conditional block between line 752 and >> line 823) on all the "shift and rotate" functions in misc.h because the >> underlying workhorse functions being called expect an "unsigned char" for >> the shift/rotate count param, but the wrapper functions themselves are >> specifying an "unsigned int" param... so when it is passed down to the inner >> funcs, it generates the warning. >> >> So, for these, just use whatever form of cast you like the best to say "hey, >> these are really unsigned chars" on the shift/rotate count param when you >> make the inner calls. :) > > Here's one of the functions (from line 755): > > template<> inline > word16 rotlFixed<word16>(word16 x, unsigned int y) > { > assert(y < 8*sizeof(x)); > return static_cast<word16>(y ? _rotl16(x, y) : x); > } > > OK, so there are two use cases here. First is the library calling it > (1). Second is external callers using it (2). > > *If* only use case (1) existed, then we could cast to silence it. But > because of use case (2), we want callers to have the benefit of > analysis.
I believe we are still not communicating... I will suggest there is no "case 1/2" - this is ALL internal to these templated "wrapper" functions - at least in my opinion. The issue with the shift/rotate count warnings is solely because the internal/workhorse/wrapped functions (e.g., _rotl16) specify *their* incoming shift/rotate count params as "unsigned char", while what is passed into them by the wrapper func is declared as an "unsigned int" - so of course this type of warning will be issued. :) But the cast to "silence" these is only operating inside of these wrappers, and so has no direct interaction with callers of the wrappers - and don't believe the inlining changes this from a code-analysis standpoint. None of this discussion is about the code sites *calling* these wrappers, whether from inside the library or outside. > We can possibly do something like: find all library callers (use case > (1)), and ensure they don't trigger the warning. Then, any warnings > that remain are user's code (use case (2)). > > I think we really need a rotate that operates on a data type larger > than a byte. We can possibly do that with inline assembly on x86. But > we won't really have a solution for x64 unless we provide MASM64 > routines in a stand alone source file (x64 lacks inline assembly). > >> Further, I might be inclined to also roll back the commit that added all the >> static_cast<word16> stuff to this block - at least if it was motivated by >> thinking that was the cause of the C4242 warnings - since the type being >> returned by the wrapped functions is already compatible with the the types >> returned by the wrapper functions. > > I think that was a different complaint. I think we are getting them > piecemeal because of the 'pragam once'. The idea was to avoid flooding > the code with all the warnings. I don't think I can say this much differently - I can only restate that I am surprised that the "static_cast<word16>" constructs added to the returned values of these wrappers were necessary for any compiler, given that the "unsigned short" return values from the wrapped functions are either the same as what "word16" is actually defined as (in MSVC-land), or at the very least, assignment-compatible with in some other compiler. And ALL of my comments are about the range in misc.h that is conditionalized as #if _MSC_VER >= 1400 && !defined(__INTEL_COMPILER) > I'll verify it once I get a Windows VM fired up. > > Jeff Cool, thanks. :) Robert -- -- You received this message because you are subscribed to the "Crypto++ Users" Google Group. To unsubscribe, send an email to [email protected]. More information about Crypto++ and this group is available at http://www.cryptopp.com. --- You received this message because you are subscribed to the Google Groups "Crypto++ Users" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
