Page MenuHomePhabricator

Fix an issue where the IgnoreName function was not allowing "Class" to be looked up inside a namespace or other class.

Authored by clayborg on Mar 27 2020, 6:12 PM.



Objective C may or may not be enabled when running expressions. There was code that was no allowing "Class" or "id" to be looked up in expressions, but it was preventing these names from being able to be looked up in a namespace or class context. This fixes the issue by passing the namespace_decl along and testing it to make sure we only stop ones at the root namespace level.

Diff Detail

Event Timeline

clayborg created this revision.Mar 27 2020, 6:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 6:12 PM
aprantl added inline comments.Mar 30 2020, 9:44 AM

For these tiny strings a StringRef == comparison is going to be more efficient than constructing and storing a pointer to a ConstString.


assertEqual(expr_result.GetValue(), None, ...) is better here, because it will print the result of expr_result.GetValue() in the failure case.


same here

shafik added a subscriber: shafik.Mar 30 2020, 10:28 AM

You also mentioned the id case failing as well. We should add that case to the test as well.

teemperor requested changes to this revision.Apr 1 2020, 12:28 AM

LGTM minus some stylistic changes.


There is no need for StringRef as ConstString allows direct comparison against string literals, so this works and is much faster: name == "id" || name == "Class"


Left over from the original test case (


I don't think we usually check the error, but only if target is valid in any other test. So this whole test can just be this (at least after D77197 has landed):

def test(self):                                                             
    """Test that we can evaluate an expression that finds something inside  
       a namespace that uses an Objective C keyword.                        
    target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))          
    self.assertTrue(target, VALID_TARGET)                                   
    self.expect_expr("a::Class x; x", result_type="a::Class")

I think new test files should follow LLVM code style.

This revision now requires changes to proceed.Apr 1 2020, 12:28 AM
clayborg updated this revision to Diff 254352.Apr 1 2020, 4:52 PM

Fixed patch reviewer suggestions and added a test for "id".

teemperor accepted this revision.Apr 21 2021, 5:50 AM

I think this review got kind of stuck. I think the only missing thing is simplifying the self.dbg.CreateTarget call in the test, but otherwise this looks good to me. I'm just going to accept this as the bit of cleanup doesn't need another revision.

(Sorry for the delay, I kept clicking on this review and just saw unaddressed comments so I thought this wasn't done yet).

This revision is now accepted and ready to land.Apr 21 2021, 5:50 AM