https://bugs.kde.org/show_bug.cgi?id=340399
--- Comment #6 from Christoph Feck <cf...@kde.org> ---
Thanks, that is already an improvement, but not ready yet.
I would have really preferred if you used our code review tools instead of
adding the patch to this feature request, but I will spare you that for now and
add my review comments here.
- The format should be extracted from a separate '--format' argument via the
arguments parser. If other xdialog type command line tools use "--getcolor
[format]" instead, please add a reference.
- QString::sprintf() is an obsolete member. Use QString::asprintf() or
QString::vasprintf() instead.
- Format strings are expected to be UTF-8. Use .toUtf8().constData() instead of
.toLocal8Bit().data()
- Your regexes do not parse '$' positional arguments (to reorder R,G,B), nor
'%o', '%i', and '%u'. Flags '#' and '-' should be handled, too.
- Length specifiers such as 'h', 'll', 'L' etc. as well as variable specifiers
'*' need to be stripped for safety. Otherwise it could reference wrong memory
when reading the values from the stack.
- You are using contains(QRegularExpression("[dDxX]")) but _all_ your regexes
contain 'd' for digits. Maybe use [0-9] explicitely.
- The format string can also contain ordinary characters before, between, and
after the values, e.g. --format="QColor(%d, %d, %d)" or
--format="#%02X%02X%02X". These need to be visible in the result.
- If possible, refactor the code to loop over number of arguments with a regexp
for each of them. This is optional for now, and not required for passing
review, but helps when alpha support is added to kdialog, which needs to handle
3 or 4 arguments for later RGB vs ARGB usage.
If you need a reference for printf-style format strings, please see
http://pubs.opengroup.org/onlinepubs/9699919799/functions/sprintf.html
--
You are receiving this mail because:
You are watching all bug changes.
