Re: Review Request 116886: Refactor private variables of KCompletion
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/#review53705 --- Ship it! Looks reasonable to me, thanks! - Frank Reininghaus On March 19, 2014, 11:01 p.m., David Gil Oliva wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/ --- (Updated March 19, 2014, 11:01 p.m.) Review request for KDE Frameworks. Repository: kcompletion Description --- Refactor private variables of KCompletion Also: reorder variables declaration to avoid padding Diffs - src/kcompletion.cpp 7396029 src/kcompletion_p.h e3fad26 Diff: https://git.reviewboard.kde.org/r/116886/diff/ Testing --- It builds. Autotests pass. Thanks, David Gil Oliva ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116886: Refactor private variables of KCompletion
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/#review53725 --- This review has been submitted with commit d7a60b8d6437090ef9009e5bfc7899d866bc39a4 by David Gil to branch master. - Commit Hook On March 19, 2014, 11:01 p.m., David Gil Oliva wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/ --- (Updated March 19, 2014, 11:01 p.m.) Review request for KDE Frameworks. Repository: kcompletion Description --- Refactor private variables of KCompletion Also: reorder variables declaration to avoid padding Diffs - src/kcompletion.cpp 7396029 src/kcompletion_p.h e3fad26 Diff: https://git.reviewboard.kde.org/r/116886/diff/ Testing --- It builds. Autotests pass. Thanks, David Gil Oliva ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116886: Refactor private variables of KCompletion
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/#review53410 --- src/kcompletion_p.h https://git.reviewboard.kde.org/r/116886/#comment37578 This is not strictly related to your changes, but it looks a bit unusual to have one plain bool and two bool bitfields next to each other. Making all bools a bitfield won't make much difference now though because the compiler will always use more memory for this class in order to preserve the 4-byte or 8-byte alignment. Another alignment-related issue is caused by your patch though: on a 64-bit system, moving the int member away from the bools will most likely increase sizeof(KCompletionPrivate) by 8 bytes because the compiler will add some padding to both in order to preserve the alignment of the neigbouring pointers. It might not make a big difference because it's probably unusual to create thousands of KCompletionPrivate instances, but still, it seems unnecessary. If one really wanted to make use of bitfields to save memory here, one could make 'order' a bitfield and move it next to the bools. - Frank Reininghaus On March 18, 2014, 11:01 p.m., David Gil Oliva wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/ --- (Updated March 18, 2014, 11:01 p.m.) Review request for KDE Frameworks. Repository: kcompletion Description --- Refactor private variables of KCompletion Also: reorder variables declaration to avoid padding Diffs - src/kcompletion_p.h e3fad26 src/kcompletion.cpp 7396029 Diff: https://git.reviewboard.kde.org/r/116886/diff/ Testing --- It builds. Autotests pass. Thanks, David Gil Oliva ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116886: Refactor private variables of KCompletion
On March 19, 2014, 11:55 a.m., Frank Reininghaus wrote: src/kcompletion_p.h, line 338 https://git.reviewboard.kde.org/r/116886/diff/2/?file=255262#file255262line338 This is not strictly related to your changes, but it looks a bit unusual to have one plain bool and two bool bitfields next to each other. Making all bools a bitfield won't make much difference now though because the compiler will always use more memory for this class in order to preserve the 4-byte or 8-byte alignment. Another alignment-related issue is caused by your patch though: on a 64-bit system, moving the int member away from the bools will most likely increase sizeof(KCompletionPrivate) by 8 bytes because the compiler will add some padding to both in order to preserve the alignment of the neigbouring pointers. It might not make a big difference because it's probably unusual to create thousands of KCompletionPrivate instances, but still, it seems unnecessary. If one really wanted to make use of bitfields to save memory here, one could make 'order' a bitfield and move it next to the bools. This is not strictly related to your changes, but it looks a bit unusual to have one plain bool and two bool bitfields next to each other. I know, but I can't change it without changing the method 'void findAllCompletions(const QString , KCompletionMatchesWrapper *matches, bool hasMultipleMatches) const' in kcompletion_p.h. The boolean parameter is passed as a reference and therefore the compiler complains it can't be a bitfield. That method is doomed to be moved elsewhere. Then I will change that bool to bitfield. If one really wanted to make use of bitfields to save memory here, one could make 'order' a bitfield and move it next to the bools. Ok, let's see if I do it well :-) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/#review53410 --- On March 18, 2014, 11:01 p.m., David Gil Oliva wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/ --- (Updated March 18, 2014, 11:01 p.m.) Review request for KDE Frameworks. Repository: kcompletion Description --- Refactor private variables of KCompletion Also: reorder variables declaration to avoid padding Diffs - src/kcompletion_p.h e3fad26 src/kcompletion.cpp 7396029 Diff: https://git.reviewboard.kde.org/r/116886/diff/ Testing --- It builds. Autotests pass. Thanks, David Gil Oliva ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116886: Refactor private variables of KCompletion
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/ --- (Updated March 19, 2014, 10:52 p.m.) Review request for KDE Frameworks. Changes --- Add TODO item for hasMultipleMatches variable. Convert order variable to bitfield, following Frank's suggestion. Repository: kcompletion Description --- Refactor private variables of KCompletion Also: reorder variables declaration to avoid padding Diffs (updated) - src/kcompletion.cpp 7396029 src/kcompletion_p.h e3fad26 Diff: https://git.reviewboard.kde.org/r/116886/diff/ Testing --- It builds. Autotests pass. Thanks, David Gil Oliva ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116886: Refactor private variables of KCompletion
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/ --- (Updated March 19, 2014, 11:01 p.m.) Review request for KDE Frameworks. Changes --- Reduce order variable bitfield. Repository: kcompletion Description --- Refactor private variables of KCompletion Also: reorder variables declaration to avoid padding Diffs (updated) - src/kcompletion.cpp 7396029 src/kcompletion_p.h e3fad26 Diff: https://git.reviewboard.kde.org/r/116886/diff/ Testing --- It builds. Autotests pass. Thanks, David Gil Oliva ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116886: Refactor private variables of KCompletion
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/ --- (Updated March 18, 2014, 11:01 p.m.) Review request for KDE Frameworks. Changes --- Forgot to commit fix. Repository: kcompletion Description --- Refactor private variables of KCompletion Also: reorder variables declaration to avoid padding Diffs (updated) - src/kcompletion_p.h e3fad26 src/kcompletion.cpp 7396029 Diff: https://git.reviewboard.kde.org/r/116886/diff/ Testing --- It builds. Autotests pass. Thanks, David Gil Oliva ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116886: Refactor private variables of KCompletion
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/#review53379 --- Ship it! Ship It! - Aleix Pol Gonzalez On March 18, 2014, 11:01 p.m., David Gil Oliva wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116886/ --- (Updated March 18, 2014, 11:01 p.m.) Review request for KDE Frameworks. Repository: kcompletion Description --- Refactor private variables of KCompletion Also: reorder variables declaration to avoid padding Diffs - src/kcompletion_p.h e3fad26 src/kcompletion.cpp 7396029 Diff: https://git.reviewboard.kde.org/r/116886/diff/ Testing --- It builds. Autotests pass. Thanks, David Gil Oliva ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
