[cmake-developers] Autogen subdirectories patches
Sebastian Holtermann
seblist at xwmw.org
Tue Jul 26 17:38:34 EDT 2016
Thanks for the feedback.
>> I have prepared an other approach which uses checksum suffixes
>> instead of nested directory generation.
>> This was tested on my Debian/amd64/Makefile setup.
>
> Interesting; I like it. Might be worthwhile to reuse it for the .o files
> in the future as well (since these can conflict, but are much less
> likely to do so).
>
>> Please review.
>
> Could you run clang-format over the code? There are a number of style
> issues that it will fix (IIRC, it needs at least Clang 3.8; I'm unsure
> what version of the clang that comes with Xcode is sufficient).
I didn't knew clang-format. That's a powerful tool!
I have applied clang-format-3.8 which is available in Debian/Stretch.
>> + rccOutputPath += cmQtAutoGeneratorUtil::BuildFileBase( qrcSourceName,
>> + makefile->GetCurrentSourceDirectory(),
>> + makefile->GetCurrentBinaryDirectory(),
>> + makefile->GetHomeDirectory(),
>> + makefile->GetHomeOutputDirectory() );
>
> Is there a reason we can't pass just the `makefile` down and have it
> query all the parameters rather than having 5 arguments?
In fact there is.
cmQtAutoGeneratorUtil::BuildFileBase is shared between
cmQtAutoGeneratorInitializer and cmQtAutoGenerators.
Both hold the directory names in different variables
which is why they must be passed manually.
>> + struct TPair {
>> + const std::string * dir;
>> + const char * seed;
>> + };
>> + TPair pDirs[4] = {
>> + { &parenDirCSource, "CurrentSource" },
>> + { &parenDirCBinary, "CurrentBinary" },
>> + { &parenDirPSource, "ProjectSource" },
>> + { &parenDirPBinary, "ProjectBinary" }
>> + };
>> + TPair * itc = &pDirs[0];
>> + TPair * ite = &pDirs[0] + ( sizeof ( pDirs ) / sizeof ( TPair ) );
>
> It might be better to just use a NULL sentinel at the end of the array.
> If not, the extra spaces here should go away.
I have changed that to use a NULL sentinel.
std::array would be the best solution IMO but it is not allowed, is it?
>> + // Calculate the file ( seed + relative path + name ) checksum
>> + std::string checksumBase64;
>> + {
>> + ::std::vector<unsigned char>hashBytes;
>
> CMake just uses std::, not ::std::.
Changed.
>> + {
>> + // Acquire hex value hash string
>> + std::string hexHash = cmCryptoHash::New("SHA256")->HashString(
>> + (sourceRelSeed+sourceRelPath+sourceFileName).c_str());
>> + // Convert hex value string to bytes
>> + hashBytes.resize ( hexHash.size()/2 );
>> + for ( unsigned int ii=0; ii != hashBytes.size(); ++ii ) {
>> + unsigned char byte ( 0 );
>> + for ( unsigned int jj=0; jj != 2; ++jj ) {
>> + unsigned char cval ( 0 );
>> + switch ( hexHash[ii*2+jj] ) {
>> + case '0': cval=0; break; case '1': cval=1; break;
>> + case '2': cval=2; break; case '3': cval=3; break;
>> + case '4': cval=4; break; case '5': cval=5; break;
>> + case '6': cval=6; break; case '7': cval=7; break;
>> + case '8': cval=8; break; case '9': cval=9; break;
>> + case 'a': cval=10; break; case 'b': cval=11; break;
>> + case 'c': cval=12; break; case 'd': cval=13; break;
>> + case 'e': cval=14; break; case 'f': cval=15; break;
>> + default: break;
>
> I feel like this is better as:
>
> int nibble = hexHash[ii * 2 + jj];
> if ('0' <= nibble && nibble <= '9') {
> cval = nibble - '0';
> } else if ('a' <= nibble && nibble <= 'f') {
> cval = nibble - 'a' + 10;
> } else {
> // internal error about an unexpected hexchar
> }
>
> Alternatively, cmUuid::IntFromHexDigit() (and possibly other methods)
> could be refactored to allow this to share an implementation.
I've changed it to the nibble version.
There also was a bit shift error (1 vs 4).
Doing so I found that Base64 allows '+' and '/' as characters which is
bad for directory names obviously.
For now these characters get replaced with 'A' and 'B'.
If this technique gets wider use using Base58 might be a better choice.
But it is not available in CMake, yet.
-Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: autogen_patch_2016-07-26-2.patch
Type: text/x-patch
Size: 18375 bytes
Desc: not available
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20160726/8208fbc1/attachment.bin>
More information about the cmake-developers
mailing list