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

s/IF_FALSE/UNLESS/ in ET_LOG macro names #8317

Open
wants to merge 1 commit into
base: gh/swolchok/237/head
Choose a base branch
from

Conversation

swolchok
Copy link
Contributor

@swolchok swolchok commented Feb 7, 2025

"do something unless X" reads much better than "do something if false X".

[ghstack-poisoned]
Copy link

pytorch-bot bot commented Feb 7, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8317

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 8ee2441 with merge base 883d33a (image):

NEW FAILURE - The following job has failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 7, 2025
swolchok added a commit that referenced this pull request Feb 7, 2025
"do something unless X" reads much better than "do something if false X".

ghstack-source-id: cae2fe0bf74e32e72179b3288bb610c1ae252afa
ghstack-comment-id: 2644043544
Pull Request resolved: #8317
Copy link
Contributor

@dbort dbort left a comment

Choose a reason for hiding this comment

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

Explicitly marking these as DEPRECATED will let us remove them in a couple of releases

do { \
if (!(cond)) { \
ET_LOG(Error, "Check failed (%s): ", #cond); \
return false; \
} \
} while (false)

/**
* Backward compatibility shim for the old name of ET_LOG_AND_RETURN_UNLESS.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Backward compatibility shim for the old name of ET_LOG_AND_RETURN_UNLESS.
* DEPRECATED: Please use ET_LOG_AND_RETURN_UNLESS instead.

do { \
if (!(cond)) { \
ET_LOG(Error, "Check failed (%s): " message, #cond, ##__VA_ARGS__); \
return false; \
} \
} while (false)

/**
* Backward compatibility shim for the old name of ET_LOG_MSG_AND_RETURN_UNLESS
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Backward compatibility shim for the old name of ET_LOG_MSG_AND_RETURN_UNLESS
* DEPRECATED: Please use ET_LOG_MSG_AND_RETURN_UNLESS instead.

@@ -338,14 +338,19 @@
*
* @param[in] cond the condition to check
*/
#define ET_LOG_AND_RETURN_IF_FALSE(cond) \
#define ET_LOG_AND_RETURN_UNLESS(cond) \
Copy link
Contributor

Choose a reason for hiding this comment

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

These feel like they should live near

#define ET_CHECK_OR_RETURN_ERROR(cond__, error__, message__, ...) \
and be called ET_CHECK_OR_RETURN_FALSE to be consistent with the core helpers. But that's a separate issue since this PR doesn't change where they live.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants