-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[analyzer][NFC] Turn NodeBuilderContext into a class #84638
[analyzer][NFC] Turn NodeBuilderContext into a class #84638
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Diego A. Estrada Rivera (diego-est) ChangesFrom issue #73088. I changed Full diff: https://github.com/llvm/llvm-project/pull/84638.diff 1 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
index 8e392421fef9bb..24d4afc551355e 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
@@ -59,7 +59,7 @@ class CoreEngine {
friend class ExprEngine;
friend class IndirectGotoNodeBuilder;
friend class NodeBuilder;
- friend struct NodeBuilderContext;
+ friend class NodeBuilderContext;
friend class SwitchNodeBuilder;
public:
@@ -194,7 +194,8 @@ class CoreEngine {
};
// TODO: Turn into a class.
-struct NodeBuilderContext {
+class NodeBuilderContext {
+public:
const CoreEngine &Eng;
const CFGBlock *Block;
const LocationContext *LC;
|
Thanks for the PR. |
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.
See my previous reply.
…ate and added the appropriate getters. Moved every member in NodeBuilderContext to private and added the following getters: - getEngine - getLocationContext Setters are not necessary since the class members are const already.
I added the appropriate functions and moved the class members into private. Additionally I ran |
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.
Alright. So we can simply mark all the members private. I was surprised a bit.
LGTM, but remove the TODO comment you fix.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
Outdated
Show resolved
Hide resolved
…vate member accesses. The files CheckerManager.h, CoreEngine.h, ExprEngine.h and CoreEngine.cpp all had bad private member accesses.
Fixed code formatting error in ternary expression.
✅ With the latest revision this PR passed the C/C++ code formatter. |
Seems like it wasn't as easy as just making them private. I think I found all the places where there were private member accesses. The tests passed on my side again and the github-pull-requests action. The code_formatter action keeps failing because of the documentation on something I didn't touch (but seems to be fixed high upstream). Everything should be fixed now. |
@diego-est Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
From issue #73088. I changed
NodeBuilderContext
into a class. Additionally, there were some other mentions of the former being a struct which I also changed into a class. This is my first time working with an issue so I will be open to hearing any advice or changes that need to be done.