[AST] NFC: Metaprogram DeclAttrOptions #16014
Conversation
|
@swift-ci please smoke test |
include/swift/AST/Attr.h
Outdated
| @@ -181,6 +181,13 @@ enum class DeclKind : uint8_t; | |||
|
|
|||
| /// Represents one declaration attribute. | |||
| class DeclAttribute : public AttributeBase { | |||
| // XXX -- We cannot use DeclKind due to layering. | |||
There was a problem hiding this comment.
I don't think we use "XXX" anywhere else in the code base. Please use "Note:" or "FIXME:" instead, depending on whether you think this should ever get fixed.
As for the actual comment, I think you should point out that it's okay if this isn't 1-to-1 with DeclKind, since it's just used to build up DeclAttrOptions. It should also probably be an enum class to not infect the DeclAttribute namespace.
There was a problem hiding this comment.
That's interesting, because I had always thought I was the first one to use "XXX" or "xxx" in that fashion. It was a hack, and I agree with @jrose-apple about not introducing it into this code base. But I would love to be able to trace down the meme. Did I copy it from somewhere and just forget? Did a chain of others copy it from me??? I know I used it in the Self VM back in the late 80's.
There was a problem hiding this comment.
It's a perfectly normal convention these days, and I've seen it in many codebases. I just don't want to add more such conventions to this codebase.
There was a problem hiding this comment.
@jrose-apple — Are you okay with HACK? What tags are acceptable? For whatever it may be worth – I suspect that some nontrivial part of the existing XXX usage in Swift is my fault.
@davidungar – I used to maintain Apple's "Darwin OS", and a part of that work, I saw XXX used in lots of old-school Unix/open-source projects. That's probably where I picked up the habit.
There was a problem hiding this comment.
HACK is around, but this isn't a hack. If it were, you should just move the DeclKind enum into its own header.
There was a problem hiding this comment.
I've elaborated the comment and switched to NOTE.
|
As an aside, @davezarzycki/@jrose-apple sounds like something that should be documented somewhere. I know we don't have a swift style guide at this point, but maybe it would make sense to add one for swift specific things? NOTE I am just asking questions, I haven't thought through all of the issues. |
Adding new Decl nodes shouldn't be harder than necessary.
davezarzycki
force-pushed
the
nfc_metaprogram_DeclAttrOptions
branch
from
8a2f085 to
0c4c02d
Compare
|
@swift-ci please smoke test |
|
@jrose-apple and @gottesmm – Food for thought for whenever the "mandatory but not enforced code review" document is written: when does review feedback border upon "nitpicking"? While I think that the quality of the source comment in this pull request improved after your feedback, I'm not sure the feedback itself was necessary. Some people are just more terse or verbose than others when they write code. And reasonable people can disagree about whether something is a |
|
I'm easier on the nitpicks on new contributors, but repeat contributors have a responsibility to not just add something but also to keep the codebase from fragmenting (further?) into many different styles and idioms. Having a second person look at your code helps keep it homogeneous between at least two people and hopefully across a larger chunk of the project. In this case I really didn't get the DeclKindIndex originally, and it took me a while to prove to myself that it didn't matter if it diverged from the real DeclKind. I probably should have swapped the two bits of feedback, though. |
|
@jrose-apple, @davidungar, @rjmccall – Ping. I think I've incorporated all review feedback. Can one of you please mark this as reviewed? Thanks! |
Adding new Decl nodes shouldn't be harder than necessary.