[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