MosheBerman planned changes to this revision. MosheBerman added a comment. Thanks for all the thoughtful and fantastic feedback!
================ Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:592-602 + // If we're inside of an NS_ASSUME, then the sourceRange will end before the + // asterisk. + const auto CanonicalTypeSize = CanonicalTypeStr.size(); + const bool IsInsideOfAssume = + NullabilityLoc == RetTypeLoc.getSourceRange().getBegin().getLocWithOffset( + CanonicalTypeSize - 1); + ---------------- steakhal wrote: > Uh, this is really ugly. I don't believe this is the preferred way of > detecting this. @NoQ WDYT? > Uh, this is really ugly. It is really ugly. And a more correct implementation would probably handle some of the edge cases highlighted in the diff summary. It’s on me - being a n00b at llvm stuff. By “_this_” do you mean `IsInsideOfAssume` or `UseSyntaxSugar` in general? > I don't believe this is the preferred way of detecting this. @NoQ WDYT? I tried `isMacroExpansion` for assume nonnull, passing the end loc of the return type without success. A colleague suggested [SourceManager::isMacroBodyExpansion](https://clang.llvm.org/doxygen/classclang_1_1SourceManager.html#a04eadd011628b7c4cd3304712c8c221c). I’ll look at it again later today. Thank you so much for your patience and direction with reviews as I work on this. I really appreciate you making time for me! ================ Comment at: clang/test/Analysis/nullability-fixits.mm:5-10 +// RUN: %clang_cc1 -analyze \ + +// RUN: -Wno-nonnull \ +// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \ +// RUN: -fdiagnostics-parseable-fixits %s 2>%t +// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-IMPLICIT ---------------- steakhal wrote: > Why don't you use the `%clang_analyze_cc1 ...` form instead or even better > the `%check_analyzer_fixit` tool-subst pattern? > See the `clang/test/Analysis/dead-stores.c` as an example. That’s a good call out. I looked at that exact test and was having trouble testing. Instead, I implemented the RUN commands from the most basic invocation I could find. I’m happy to update the tests that verify that fixits are emitted. For the one that checks that fixits are applied, is there a better way? The `-verify` flag doesn’t do that, from what I saw in the docs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123352/new/ https://reviews.llvm.org/D123352 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
