Skip to content

Conversation

@atrick
Copy link
Contributor

@atrick atrick commented Apr 11, 2019

This is one of the most common operations that I perform in a debugger.

This is one of the most common operations that I perform in a debugger.
@atrick atrick requested a review from gottesmm April 11, 2019 18:42
@atrick
Copy link
Contributor Author

atrick commented Apr 11, 2019

@swift-ci smoke test and merge.

@atrick
Copy link
Contributor Author

atrick commented Apr 11, 2019

@swift-ci smoke test linux

return bool(ApplySiteKind::fromNodeKind(inst->getKind()));
}

void dump() const { getInstruction()->dump(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you wrap this with a used attribute and an obsolute attribute. E.x.:

LLVM_ATTRIBUTE_DEPRECATED(void dump() const, "only for use in the debugger");

Copy link
Contributor

Choose a reason for hiding this comment

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

(Forgot to add the LLVM_ATTRIBUTE_USED). So it would actually bt his:

LLVM_ATTRIBUTE_DEPRECATED(void dump() const LLVM_ATTRIBUTE_USED, "only for use in the debugger");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't use LLVM_ATTRIBUTE_DEPRECATED because it's not a declaration, it's a trivial inline function definition around SILInstruction::dump() and I didn't want to declare it separately. Incidentally, SILInstruction::dump() is not declared as LLVM_ATTRIBUTE_DEPRECATED either, and it's expected to call these functions within the source to generate debug output.

LLVM_ATTRIBUTE_USED makes perfect sense.

I frankly don't understand the use-case for using both of these attributes together. What problem is solved by polluting the code with LLVM_ATTRIBUTE_DEPRECATED? Sometimes people (like me) accidentally commit a wayward debug print call that's not under a flag for a short time, but I don't see that as a serious problem that the attribute can solve in the common case.

@atrick
Copy link
Contributor Author

atrick commented Apr 11, 2019

This already passed full testing.

@atrick
Copy link
Contributor Author

atrick commented Apr 16, 2019

@swift-ci smoke test and merge.

@atrick
Copy link
Contributor Author

atrick commented Apr 16, 2019

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit 78b4ab3 into swiftlang:master Apr 16, 2019
@atrick atrick deleted the add-applysite-dump branch May 8, 2019 21:35
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.

3 participants