https://bugs.kde.org/show_bug.cgi?id=340399
--- Comment #7 from yuehangwu <yuehan9...@gmail.com> --- Hi Christoph, firstly thanks for the above advice. (In reply to Christoph Feck from comment #6) > 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. OK, is https://git.reviewboard.kde.org the right place? > - 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. will revise the regx to suite more specifiers > - You are using contains(QRegularExpression("[dDxX]")) but _all_ your > regexes contain 'd' for digits. Maybe use [0-9] explicitely. I think [d] is only matched when your specifier is char 'd' instead of \d. and when I feed "%0002d" to sprintf, I got warned. That's why I choose it as [1-9] for decimal case. right? > - 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. OK, that means release "^" and "$" check for head and tail. > - 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. sounds like use "(%[1-9]+\\.?\\d*d|%d)" to loop over the arguments. -- You are receiving this mail because: You are watching all bug changes.
