-
Notifications
You must be signed in to change notification settings - Fork 122
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
Pass the DiffRequest down to the visitors. NFC. #933
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 28. Check the log or trigger a new build to see more.
@@ -99,11 +99,11 @@ namespace clad { | |||
/// A base class for all common functionality for visitors | |||
class VisitorBase { | |||
protected: | |||
VisitorBase(DerivativeBuilder& builder) | |||
VisitorBase(DerivativeBuilder& builder, const DiffRequest& request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: constructor does not initialize these fields: m_Mode [cppcoreguidelines-pro-type-member-init]
VisitorBase(DerivativeBuilder& builder, const DiffRequest& request)
^
/// The function that is currently differentiated. | ||
const clang::FunctionDecl* m_Function; | ||
/// The differentiation request that is being currently processed. | ||
const DiffRequest& m_DiffReq; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member 'm_DiffReq' of type 'const DiffRequest &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]
const DiffRequest& m_DiffReq;
^
/// The function that is currently differentiated. | ||
const clang::FunctionDecl* m_Function; | ||
/// The differentiation request that is being currently processed. | ||
const DiffRequest& m_DiffReq; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'm_DiffReq' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
const DiffRequest& m_DiffReq;
^
DeclarationNameInfo name(II, validLoc); | ||
llvm::SaveAndRestore<DeclContext*> SaveContext(m_Sema.CurContext); | ||
llvm::SaveAndRestore<Scope*> SaveScope(getCurrentScope()); | ||
DeclContext* DC = const_cast<DeclContext*>(m_Function->getDeclContext()); | ||
DeclContext* DC = const_cast<DeclContext*>(m_DiffReq->getDeclContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
DeclContext* DC = const_cast<DeclContext*>(m_DiffReq->getDeclContext());
^
The intent of this patch is to centralize the information about the diff request in one place. This will help upcoming patches to reduce the state of the visitors and refactor some of the implementation in a common place. That will unblock work on #721.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 17. Check the log or trigger a new build to see more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
6bc7d9e
to
97d7051
Compare
This patch avoids picking up the latest uploaded llvm18 and partially mixing it with llvm17.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
The intent of this patch is to centralize the information about the diff request in one place. This will help upcoming patches to reduce the state of the visitors and refactor some of the implementation in a common place.
That will unblock work on #721.