Compiled and run the unittests
Details
Details
Diff Detail
Diff Detail
- Repository
- R237 KConfig
- Branch
- rangeBasedFor
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 20096 Build 20114: arc lint + arc unit
Comment Actions
The patch being large I didn't check each one separately but I suspect this patch misses boat loads of qAsConst. Besides there's at least one case of very flawed logic, it can't be about blindly switching one loop construct for another especially when the iterators are compared inside the loop body.
| src/kconfig_compiler/kconfig_compiler.cpp | ||
|---|---|---|
| 2119 | This logic is very flawed... instead of comparing positions in the list now you're comparing actual values. If two different positions in the list would end up having the same values (for some reason) we'd generate syntactically incorrect code. | |
Comment Actions
Yes, it indeed misses, I'll add the qAsConst. ( I would actually prefer to change the QList to std::vector in the future, no need to add the 'qAsConst' there and it would help readability )
Besides there's at least one case of very flawed logic, it can't be about blindly switching one loop construct for another especially when the iterators are compared inside the loop body.
I didn't blindly change anything, everything I did I tested either by running the unittests or recompilling and testing the settings of applications.
This patch greatly increases the readability of the software. I'll do another round of reviews here adding the qAsConst and trying to find broken logic but I don't believe they are really broken.
| src/kconfig_compiler/kconfig_compiler.cpp | ||
|---|---|---|
| 2119 | if two items in the list have the same values the KConfig XML is wrong and the generated code will not compile. I don't see an issue with it. | |
Comment Actions
Sure I know you did this, otherwise I think I'd call you criminal. ;-)
More seriously, I find the test suite of kconfig_compiler unfortunately very slim on error cases (as in XML with crap in them) so that's why we should try to keep in mind "creative" users during reviews. As a corollary, the absence of failing tests doesn't mean the absence of bugs being introduced.
As I'm trying to highlight in my comment below, I think that in that particular case moving away from iterators introduce a regression in behavior and information provided to the user in case of bogus data.
| src/kconfig_compiler/kconfig_compiler.cpp | ||
|---|---|---|
| 2119 | Well, there's a difference between generating malformed code and generating well formed code which doesn't compile for grammar reasons. Previously it wouldn't compile with a proper error (two parameters with the same name) now it would in most case give a totally unrelated error since the tokenizer would go haywire. | |