[cfe-commits] [Patch]

[Douglas Gregor]

On Sep 19, 2011, at 8:50 AM, erikjv wrote:

> Renewed attempt, this time against rev. #140017

A few more comments…

--- a/lib/Sema/SemaAccess.cpp
+++ b/lib/Sema/SemaAccess.cpp
@@ -1642,6 +1642,24 @@ void Sema::CheckLookupAccess(const LookupResult &R) {
   }
 }
 
+bool Sema::IsSimplyAccessible(NamedDecl *decl, CXXRecordDecl *Class) {
+  if (!Class)
+    return true;
+
+  if (!ValueDecl::classof(decl))
+    return true;

The canonical way to check this would be isa<ValueDecl>(decl), but why are you checking this at all? Type declarations can be private/protected just like values can.

Please add a comment block describing what this function does.

index 5b44841..47add1c 100644
--- a/lib/Sema/SemaCodeComplete.cpp
+++ b/lib/Sema/SemaCodeComplete.cpp
@@ -1186,8 +1186,14 @@ namespace {
     CodeCompletionDeclConsumer(ResultBuilder &Results, DeclContext *CurContext)
       : Results(Results), CurContext(CurContext) { }
     
-    virtual void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, bool InBaseClass) {
-      Results.AddResult(ND, CurContext, Hiding, InBaseClass);
+    virtual void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
+                           bool InBaseClass) {
+      bool Accessible = true;
+      if (Ctx)
+        if (CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(Ctx))
+            Accessible = Results.getSema().IsSimplyAccessible(ND, Class);
+      ResultBuilder::Result Result(ND, 0, false, Accessible);
+      Results.AddResult(Result, CurContext, Hiding, InBaseClass);

Could you add a FIXME: here to check accessibility for Objective-C class members? You're not on the hook for making it work with Objective-C, but I'd rather not forget that we want to do it.

--- /dev/null
+++ b/test/Index/complete-access-checks.cpp
@@ -0,0 +1,59 @@
+struct X {
+  int member1;
+  void func1();
+protected:
+  int member2;
+  void func2();
+private:
+  int member3;
+  void func3();
+};
+
+struct Y: protected X {
+  void doSomething();
+};
+
+class Z {
+public:
+  int member1;
+  void func1();
+protected:
+  int member2;
+  void func2();
+private:
+  int member3;
+  void func3();
+};

The fun case to test would be something like this:

class X {
protected:
  int member;
};

class Y : public X {
public:
  using X::member;
};

void f(X x, Y y) {
  x.<complete> // member is inaccessible
  y.<complete> // member is accessible
}

which would help determine whether you're handling using declarations (== the hard case) correctly.

	- Doug

> -- Erik.
> 
> On 15 Sep, 2011,at 05:23 PM, Douglas Gregor <dgregor at apple.com> wrote:
> 
>> 
>> On Sep 14, 2011, at 9:31 AM, Erik Verbruggen wrote:
>> 
>> > Hello,
>> > 
>> > Attached is a patch to add CXAvailability_NotAccessible to indicate that a declaration is available, but not accessible from the current code completion context. Included is a test-case which checks this with public/protected/private members/methods for C++. This can be used with code-completion to indicate availability/accessibility of completed items.
>> > 
>> > The patch is against trunk rev. #139681. As always, feedback is welcome.
>> 
>> Cool! A few comments:
>> 
>> diff --git a/include/clang/Sema/Lookup.h b/include/clang/Sema/Lookup.h
>> index ce762b8.0f7dfea 100644
>> --- a/include/clang/Sema/Lookup.h
>> +++ b/include/clang/Sema/Lookup.h
>> @@ -655,7 +655,7 @@ private:
>> /// \param InBaseClass whether this declaration was found in base
>> /// class of the context we searched.
>> virtual void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, 
>> - bool InBaseClass) = 0;
>> + bool InBaseClass, bool Accessible = true) = 0;
>> };
>> 
>> 
>> I'm not thrilled about adding the Accessible flag here, because I don't want to pay the cost of access checking for every declaration we visit with this generic interface. For example, spell-checking shouldn't pay the cost of access checking for every declaration, because spell-checking only ends up examining a very small number of declarations for further consideration.
>> 
>> --- a/include/clang/Sema/Sema.h
>> +++ b/include/clang/Sema/Sema.h
>> @@ -3648,6 +3648,7 @@ public:
>> bool ForceCheck = false,
>> bool ForceUnprivileged = false);
>> void CheckLookupAccess(const LookupResult &R);
>> + bool IsAccessible(NamedDecl *decl, CXXRecordDecl *Class, bool isUsingDecl);
>> 
>> Can the name of this routine have some sort of "Simple" modifier tacked on to it? Full access-checking involves a bunch of madness in member access expressions and such, which we aren't doing here.
>> 
>> --- a/tools/c-index-test/c-index-test.c
>> +++ b/tools/c-index-test/c-index-test.c
>> @@ -237,6 +237,10 @@ static void PrintCursor(CXTranslationUnit TU, CXCursor Cursor) {
>> case CXAvailability_NotAvailable:
>> printf(" (unavailable)");
>> break;
>> +
>> + case CXAvailability_NotAccessible:
>> + printf(" (unaccessible)");
>> + break;
>> }
>> 
>> if (clang_CXXMethod_isStatic(Cursor))
>> @@ -1050,6 +1054,10 @@ void print_completion_result(CXCompletionResult *completion_result,
>> case CXAvailability_NotAvailable:
>> fprintf(file, " (unavailable)");
>> break;
>> +
>> + case CXAvailability_NotAccessible:
>> + fprintf(file, " (unaccessible)");
>> + break;
>> }
>> fprintf(file, "\n");
>> }
>> -- 
>> 1.7.4.4
>> 
>> "inaccessible" rather than "unaccessible", please
>> 
>> - Doug
>> 
> <0001-Added-CXAvailability_NotAccessible-to-indicate-that-.patch>