[PATCH] D16748: Cleanup MemRegion.cpp/MemRegion.h

Alexander Riccio via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 2 17:15:02 PST 2016


ariccio marked 3 inline comments as done.
ariccio added a comment.

I will remove that `const` later tonight.


================
Comment at: C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1396
@@ -1395,3 +1395,3 @@
   const auto &ReferencedBlockVars = AC->getReferencedBlockVars(BC->getDecl());
-  auto NumBlockVars =
+  const auto NumBlockVars =
       std::distance(ReferencedBlockVars.begin(), ReferencedBlockVars.end());
----------------
aaron.ballman wrote:
> ariccio wrote:
> > ariccio wrote:
> > > aaron.ballman wrote:
> > > > No need for a const here; the correct change is to not use auto (here and the line above), but instead spell out the type explicitly.
> > > The const is partly to make sure that the `if (NumBlockVars == 0) {` line never accidentally becomes `if (NumBlockVars = 0) {` like it did in CPython:
> > > 
> > > http://bugs.python.org/issue25844
> > For archival reasons, I'll copy & paste the relevant diff here (I hate dead links):
> > 
> > 
> > ```
> > --- a/PC/launcher.c
> > +++ b/PC/launcher.c
> > @@ -114,7 +114,7 @@ static wchar_t * get_env(wchar_t * key)
> >      if (result >= BUFSIZE) {
> >          /* Large environment variable. Accept some leakage */
> >          wchar_t *buf2 = (wchar_t*)malloc(sizeof(wchar_t) * (result+1));
> > -        if (buf2 = NULL) {
> > +        if (buf2 == NULL) {
> >              error(RC_NO_MEMORY, L"Could not allocate environment buffer");
> >          }
> >          GetEnvironmentVariableW(key, buf2, result);
> > ```
> While this form of bug can certainly crop up, it's still a bridge-too-far for this project, as I understand it our de facto guidelines on this. I am not certain that we want to start sprinkling const onto value types (as opposed to reference and pointer types) at this point. If we do, it should certainly be something handled a bit more consistently and actively than a general clean-up related to unnecessary type casting.
> If we do, it should certainly be something handled a bit more consistently and actively than a general clean-up related to unnecessary type casting.

You've got a good point there. I'll upload a final revision without that `const` a bit later tonight.



http://reviews.llvm.org/D16748





More information about the cfe-commits mailing list