> On June 29, 2015, 8:40 a.m., David Faure wrote: > > src/karchive.cpp, line 684 > > <https://git.reviewboard.kde.org/r/124165/diff/3/?file=381630#file381630line684> > > > > The "/8 /8 %2" bit looks too arithmetic to me, this should be 'bitwise > > and' instead (faster, more approriate here, and IMHO more readable). > > We can't use S_IXUSR(perms), S_IXGRP(perms) and S_IXOTH(perms) due to > > windows portability, so I would suggest this instead: > > > > if (perm & 01) > > ret |= QFileDevice::ExeOther; > > if (perm & 010) > > ret |= QFileDevice::ExeGroup; > > if (perm & 0100) > > ret |= QFileDevice::ExeOwner; > > > > Also, QFileDevice::Permissions is a QFlags, so no casting needed. > > Declare > > QFileDevice::Permission ret = filePerms; > > and "return ret;" > > Romário Rios wrote: > > Also, QFileDevice::Permissions is a QFlags, so no casting needed. > Declare > > QFileDevice::Permission ret = filePerms; > > and "return ret;" > > About that...: > > ``` > [ 31%] Building CXX object src/CMakeFiles/KF5Archive.dir/karchive.cpp.o > /home/luizromario/karchive/src/karchive.cpp: In function > 'QFileDevice::Permission withExecutablePerms(QFileDevice::Permissions, > mode_t)': > /home/luizromario/karchive/src/karchive.cpp:682:35: error: invalid > conversion from 'QFlags<QFileDevice::Permission>::Int {aka int}' to > 'QFileDevice::Permission' [-fpermissive] > /home/luizromario/karchive/src/karchive.cpp:685:27: error: invalid > conversion from 'int' to 'QFileDevice::Permission' [-fpermissive] > /home/luizromario/karchive/src/karchive.cpp:688:27: error: invalid > conversion from 'int' to 'QFileDevice::Permission' [-fpermissive] > /home/luizromario/karchive/src/karchive.cpp:691:27: error: invalid > conversion from 'int' to 'QFileDevice::Permission' [-fpermissive] > ```
My bad, I forgot the 's' in my suggested code. It's QFileDevice::Permissions ret = filePerms; - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124165/#review81846 ----------------------------------------------------------- On June 28, 2015, 10:31 a.m., Romário Rios wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124165/ > ----------------------------------------------------------- > > (Updated June 28, 2015, 10:31 a.m.) > > > Review request for KDE Frameworks. > > > Repository: karchive > > > Description > ------- > > The KArchiveFile::copyTo() doesn't set the permissions of the files it copies > -- it just copies the data. So if an archive contains an executable, it's > extracted from the archive as a non-executable. This patch makes it apply the > executable permissions found in the KArchiveFile to the QFile being copied. > > > Diffs > ----- > > autotests/karchivetest.cpp afc8387 > src/karchive.cpp 344cba8 > > Diff: https://git.reviewboard.kde.org/r/124165/diff/ > > > Testing > ------- > > Seems to work for all files (tested with xz and zip files containing > executables, directories, text, etc.). > > > Thanks, > > Romário Rios > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel