MosheBerman marked 3 inline comments as done. MosheBerman added inline comments.
================ 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); + ---------------- MosheBerman wrote: > 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! Hello again! I tried a few things and looked at some of the clang source and I think this is the only way to check for this right now. The type system treats nullability attributes as Objective-C syntax sugar. It adds attributes on in SemaType.cpp, but does not track if an attribute is inferred by `NS_ASSUME` macros or if it's spelled-out. Consequentially, clang also does not preserve any information about the macro ID for the type. The reason this works right now is because the pointer character isn't included in `NullabilityLoc`. We can fix this by setting `isImplicit` on the inferred `Attr`s in SemaType.cpp, but I'm unsure if that's the correct approach. For now, this continues to work, and the tests will catch any breaking changes. 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
