[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Really good progress. Nicestuff. I really appreciate the unittest.
However Ive got some minor comments there :D



================
Comment at: clang/test/Analysis/fixed-point.c:9
+
+enum en_t { en_0 = 1 };
+
----------------
I would suggest 'Kind' or something similar. So by reading it would feel natural that is an enum.
Its probably a personal preference though. Disregard if you want.


================
Comment at: clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp:1
+//===- unittest/StaticAnalyzer/AnalyzerOptionsTest.cpp - SA Options test --===//
+//
----------------
Copypaste typo: SA Options?. Check the rest.


================
Comment at: clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp:13
+#include "clang/Basic/TargetInfo.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
----------------
Do we really need checkers here? Check the rest of the includes. It will reduce compile times inthe long run.


================
Comment at: clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp:20
+
+namespace clang {
+namespace ento {
----------------
Should the content really be inside the ento namespace?
I would probably advocate for anonymous namespaces instead.


================
Comment at: clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp:23
+
+TEST(getAPSIntTypeTest, foo) {
+  std::unique_ptr<ASTUnit> AST = tooling::buildASTFromCode("");
----------------
Give a meaningful name for this.


================
Comment at: clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp:29
+
+  APSIntType ty = BVF.getAPSIntType(Context.LongAccumTy);
+  EXPECT_TRUE(ty.getBitWidth() == Context.getTargetInfo().getLongAccumWidth());
----------------
We use Upper Camel Case for variables as per llvm. In addition we usually refer to QualTypes by 'Ty'.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139759/new/

https://reviews.llvm.org/D139759