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

Clean up bcc_exception.h #824

Merged
merged 3 commits into from
Nov 30, 2016
Merged

Clean up bcc_exception.h #824

merged 3 commits into from
Nov 30, 2016

Conversation

palmtenor
Copy link
Member

@palmtenor palmtenor commented Nov 29, 2016

  • Remove unused headers and Macros.
  • Implement a StatusTuple class that has code() and msg() method, instead of using std::get and std::tuple, as suggested by @4ast and @drarmstr in C++ helper class for BCC #781.
  • clang-format -i on the file.

Copy link
Member

@4ast 4ast left a comment

Choose a reason for hiding this comment

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

probably makes sense to convert StatusTuple to proper class and add code()/msg() methods.

@palmtenor
Copy link
Member Author

Updated according to comment

return __status; \
} \
} while (0)
static int status_code(const StatusTuple &status_tuple) {
Copy link
Member

Choose a reason for hiding this comment

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

since it's in the header file, it probably needs 'static inline' ?

@palmtenor
Copy link
Member Author

palmtenor commented Nov 30, 2016

Should I rename mkstatus_ in frontends/b/node.h to be mkstatus? I guess originally the underscore is to avoid name conflict with the mkstatus functions from exception.h. But we don't have those anymore...

@4ast 4ast merged commit 8ad9ba0 into iovisor:master Nov 30, 2016
@4ast
Copy link
Member

4ast commented Nov 30, 2016

you could have kept mkstatus() as inline func that returns StatusTuple, but since you've changed all of the callsites anyway, I think it's fine.

@palmtenor palmtenor deleted the exception_h branch November 30, 2016 03:17
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.

2 participants