Skip to content

Conversation

@OscarSwanros
Copy link
Contributor

@OscarSwanros OscarSwanros commented Oct 26, 2017

I'm trying to check if the expression is either a MemberRefExpr or a DeclRefExpr, as per @jrose-apple's comment on the ticket. However, I don't think this is the right place/way to do it. Opened the PR just to keep track of things and maybe get some pointers from people more familiar with the matter.

Checks for a DeclRefExpr or a MembeRefExpr in 8d58457 now

Resolves #48759.

@OscarSwanros OscarSwanros changed the title WIP: [SEMA] SR-6207 [SEMA] SR-6207: Fix misleading error for unqualified use of static variable from instance method Oct 27, 2017
@OscarSwanros
Copy link
Contributor Author

I think this is ready for review.

"static member %1 cannot be used on instance of type %0",
(Type, DeclName))
ERROR(could_not_use_type_member_on_instance_without_name,none,
"static element %0 cannot be referenced as an instance member",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: static member is right here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we’re at it, this doesn’t seem much clearer than what was there before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just following @jrose-apple's:

The starter bug form of this would be to only include the name if the base expression was a simple DeclRefExpr, or perhaps also a MemberRefExpr

I'm open to suggestions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it's clearer because it directly contrasts static with instance members without any extraneous information, but agree that static member is the correct term.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my "only" was ambiguous! I meant "this should be checked in all circumstances, but don't worry about printing the name of the member if it's not easy to do so". I like Huon's suggested wording in the bug report.

@@ -1,3 +1,4 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Extra whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the original whitespace at the top but Github is confused about it.

@OscarSwanros
Copy link
Contributor Author

My laptop is going for keyboard repair, so I may be unable to code some more here. Anyone has any further comments or a pointer on how to move forward?

@CodaFi
Copy link
Contributor

CodaFi commented Nov 14, 2019

The diagnostics model has moved on since this was written. I'm trying to rebase this locally, but there's a merge conflict on pretty much every commit now. Could you please resubmit this after having rebased it and updated to the new model? If you need some help with that, please let me or @xedin know in SR-6207.

Sorry for not reviewing this sooner!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants