Skip to content

Conversation

@SuperWig
Copy link
Contributor

@SuperWig SuperWig commented Jul 22, 2021

Continuation of #2072. I kept getting issues with the uppercase name of the branch so I renamed it. Didn't think it would close the PR 😕.

Fixes #306

@SuperWig
Copy link
Contributor Author

That is almost all of them now. Will finish up tomorrow along with fixing my inevitable mistakes.

@CaseyCarter CaseyCarter added the documentation Related to documentation or comments label Jul 22, 2021
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

This is mostly looking good, just a few places where the comment was GARBAGE name [other stuff..] for which I think the [other stuff...] was just barely useful enough to keep.

@SuperWig
Copy link
Contributor Author

SuperWig commented Jul 23, 2021

I think that's it? Might have removed some useful ones in that last batch.

As far as I can tell the only remaining shouty comments are useful? The ones in stl/src/xmath.hpp and the ones in stl/inc/yvals.h (found searching "PROPERTIES").

Edit: Forgot to save atomic.cpp so the one at the top is also remaining.

@CaseyCarter
Copy link
Contributor

Hopefully got them all now I have left some in yvals(_core).h as they might be helpful?

I prefer to err on the side of keeping things, so if you think they might be useful, let's keep'em. As large as this set of changes is, it's best to restrict it to changes that are as uncontroversial as possible. Anything questionable or that might need more attention / surgery we can do elsewhere which avoids blowing this up into something that takes multiple days to review.

Also - are you ready to make this a non-Draft PR?

@SuperWig
Copy link
Contributor Author

so if you think they might be useful

Only as they're blocks of macros and searching for the banner might be slightly easier? And yes, was just going to wait for an answer to that question.

@SuperWig SuperWig marked this pull request as ready for review July 23, 2021 17:48
@SuperWig SuperWig requested a review from a team as a code owner July 23, 2021 17:48
@fsb4000
Copy link
Contributor

fsb4000 commented Jul 24, 2021

add

Fixes #306

to your first comment.

Copy link
Contributor

@AlexGuteniev AlexGuteniev left a comment

Choose a reason for hiding this comment

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

I think some macro comments should be preserved. They give non obvious context, since macros lack type system, and these particular are not even verbose.

@StephanTLavavej StephanTLavavej self-assigned this Jul 28, 2021
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks - I'll go ahead and push minor changes.

@StephanTLavavej
Copy link
Member

Ok, I've pushed a merge with main (trivial to resolve - recent PRs added/altered code around stable name removals) and the following:

  • Restore stable names for optional and variant.
  • In variant/optional, drop more shouty banners but keep stable names.
  • Remove many more shouty banners.
  • Rework shouty banners to preserve info.

In optional and variant I'd be happy to remove those stable name citations entirely, but as some were already present with lowercase comments like:

// assignment [variant.assign]

I felt it was more consistent to keep them for now.

@StephanTLavavej StephanTLavavej removed their assignment Aug 4, 2021
@StephanTLavavej
Copy link
Member

Removed the optional/variant stable name comments (including the ones of the form // assignment [optional.object.assign]) after @CaseyCarter confirmed they weren't desired.

@StephanTLavavej StephanTLavavej self-assigned this Aug 5, 2021
@StephanTLavavej StephanTLavavej merged commit 4721611 into microsoft:main Aug 6, 2021
@StephanTLavavej
Copy link
Member

THANKS FOR CLEANING UP THESE COMMENTS! 😹

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

Labels

documentation Related to documentation or comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

STL: WE SHOULD REMOVE THE SHOUTY COMMENT BANNERS!

5 participants