This is an archive of the discontinued LLVM Phabricator instance.

Fix buildbot breakage after r253106
ClosedPublic

Authored by chying on Nov 13 2015, 6:57 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

chying updated this revision to Diff 40196.Nov 13 2015, 6:57 PM
chying retitled this revision from to Fix buildbot breakage after r253106.
chying updated this object.
chying added a reviewer: jingham.
chying added a subscriber: lldb-commits.

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 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

jingham edited edge metadata.Nov 16 2015, 11:10 AM

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.

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.

chying updated this revision to Diff 40347.Nov 16 2015, 2:42 PM
chying edited edge metadata.

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.

Ahh, sorry. I litearlly just committed a change to the same function. You may have to do a merge, sorry about that.

Ahh, I see, will merge again.

chying updated this revision to Diff 40353.Nov 16 2015, 3:26 PM

Rebase on the latest commit.

zturner accepted this revision.Nov 16 2015, 3:42 PM
zturner added a reviewer: zturner.
This revision is now accepted and ready to land.Nov 16 2015, 3:42 PM
This revision was automatically updated to reflect the committed changes.

Thanks for dealing with this!