Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: Drop #warning TODO #17012

Merged
merged 1 commit into from Aug 15, 2017
Merged

Conversation

joscollin
Copy link
Member

@joscollin joscollin commented Aug 14, 2017

Drop "#warning TODO" from rgw_crypt.cc, as the tracker is created for this requirement and the
corresponding PR is in progress for a long time.

Tracker: http://tracker.ceph.com/issues/19851
PR: #14498

The below warning is displayed during build:

ceph/src/rgw/rgw_crypt.cc:38:2: warning: #warning "TODO: move this code to auth/Crypto for others to reuse." [-Wcpp]
 #warning "TODO: move this code to auth/Crypto for others to reuse."
  ^
ceph/src/rgw/rgw_crypt.cc:247:2: warning: #warning "TODO: use auth/Crypto instead of reimplementing." [-Wcpp]
 #warning "TODO: use auth/Crypto instead of reimplementing."
  ^

Signed-off-by: Jos Collin jcollin@redhat.com

Drop "#warning TODO" from rgw_crypt.cc,  as the tracker is created for this requirement and the
corresponding PR is in progress for a long time.

Tracker: http://tracker.ceph.com/issues/19851
PR: ceph#14498

Signed-off-by: Jos Collin <jcollin@redhat.com>
@@ -35,8 +35,6 @@ using namespace rgw;
/**
* Encryption in CTR mode. offset is used as IV for each block.
*/
#warning "TODO: move this code to auth/Crypto for others to reuse."

Choose a reason for hiding this comment

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

warnings were added to move the code to auth/Cryto.
But Have you raised an PR for movement?
Or any existing PR that has done movement.
I believe class AES_256_CTR needed to be moved .. but not done yet

Copy link
Member Author

@joscollin joscollin Aug 14, 2017

Choose a reason for hiding this comment

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

@amitkumar50 Please read the PR description above for the PR details for the movement.

First of all, adding warnings like this is not a good practise and the TODO should be specified in a tracker. Not within the code itself.

This PR is created as the PR #14498 takes too long to get merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

@amitkumar50
In addition to my previous comment, it is ridiculous to purposefully create warnings for the TODO comments in the code.

Copy link
Member

Choose a reason for hiding this comment

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

maybe @aclamk remembers why the #warnings, I vaguely remember it might have been @liewegas that asked to do it. Anyhow, it'd be better to revert 326a16d.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yehudasa Yeah, that may be true. But it's been annoying nowadays as it appears in every other PR builds.

Copy link
Member

Choose a reason for hiding this comment

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

@joscollin the advantage if you revert the commit is we have the warning retained as comment and helps us trace back git histories better

Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

We already have a ticket for doing the work, and this warning needs to go away — it's occasionally hiding real warnings from people.

Reviewed-by: Greg Farnum gfarnum@redhat.com

@gregsfortytwo gregsfortytwo merged commit c834140 into ceph:master Aug 15, 2017
@joscollin joscollin deleted the wip-rgw-todo-warning branch August 16, 2017 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants