- expectedFailureAll takes single compiler name rather than compiler list because it also takes version which only apply to single compiler
- Don't try to access self.debug_info if debug_info is None
- Link to broken build, http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/8441
Details
- Reviewers
zturner jingham - Commits
- rG0c35282c659b: Re-Apply "Add a "not_in()" function you can apply to the list type arguments to…
rLLDB253272: Re-Apply "Add a "not_in()" function you can apply to the list type arguments to…
rL253272: Re-Apply "Add a "not_in()" function you can apply to the list type arguments…
Diff Detail
- Repository
- rL LLVM
Event Timeline
I think the is None check should go back inside the check_list_or_lambda function. Bringing it outside the function looks identical to me. If None is passed to the function, the callable() check will fail, and then the else branch will return true.
Also the compiler check is wrong currently because of the grouping of parentheses. For example, if someone passes compiler=None and compiler_version=3.7, then check would succeed no matter what the compiler version was.
Ahh ok I see the problem. Sorry I didn't read your description. Let me think about it some more
I guess this is ok. I don't like raising the None check outside the function, but I don't see another way that isn't more convoluted.
However, I think you still need to fix the grouping of the parentheses. the compiler is None should not override the version check.
I thought a call of expectedFailureAll(..compiler=None, compiler_version=['>=', '3.7']...) doesn't make much sense at the first place, so maybe just ignore compiler check in such cases?
Do you suggest to check current compiler(no matter clang or gcc or others) version against given version?
Makes sense. If compiler is None, but compilerVersion is None, maybe you could just assert. Because that's an error in the use of the decorator IMO
If the problem is that the self of the test object doesn't always have a debug_info setting, why not just always initialize it to None? Seems weird to have a general property like this that we don't initialize.
Gotta love Python. But yea, that seems like a reasonable solution as well, and makes the code cleaner.
I'm still unsure if the change to the compiler here check was the result of a failure or just a drive-by change. It always has a getCompiler() method right? So why was that particular change needed? Maybe it wasn't?
The compiler None check is because inside of check_list_or_lambda function, it checks for "value in list_or_lamda", which will throw exception if value is None
I agree that the code will be cleaner if debug_info is initialized to None.
Will try to upload another patch soon.
Recommit r253106 - Add a "not_in()" function you can apply to the list type arguments to expectedFailureAll ...
Initialize self.debug_info in Base::setUp()
Check for None before calling "value in list"
Ahh, sorry. I litearlly just committed a change to the same function. You may have to do a merge, sorry about that.