ziqingluo-90 added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:943 +def warn_pragma_unsafe_buffer_usage_misorder : +ExtWarn<"pragma unsafe_buffer_usage used in misorder">, + InGroup<IgnoredPragmas>; ---------------- NoQ wrote: > IIUC `ExtWarn` means it's a warning of the form "warning: XXX is a language > extension". It's not just a warning that has something to do with language > extensions, it's a warning that tries to warn the user about the very fact > that it's an extension. So we should just use `Warning` or `Error`. > > Also about wording: we already have a somewhat similar `#pragma > assume_nonnull`, can we use similar text? And probably prefer `Error` because > that's what the other pragma uses too: > ``` > def err_pp_assume_nonnull_syntax : Error<"expected 'begin' or 'end'">; > def err_pp_double_begin_of_assume_nonnull : Error< > "already inside '#pragma clang assume_nonnull'">; > def err_pp_unmatched_end_of_assume_nonnull : Error< > "not currently inside '#pragma clang assume_nonnull'">; > def err_pp_include_in_assume_nonnull : Error< > "cannot %select{#include files|import headers}0 " > "inside '#pragma clang assume_nonnull'">; > def err_pp_eof_in_assume_nonnull : Error< > "'#pragma clang assume_nonnull' was not ended within this file">; > ``` Fixed! ================ Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2379 + for (auto UnsafeUse : UnsafeUses) + if (!DE.isSafeBufferOptOut(SM, UnsafeUse->getBeginLoc())) + UnsafeUsesToReport.push_back(UnsafeUse); ---------------- jkorous wrote: > NoQ wrote: > > I believe this check should be performed *much earlier*. It's not about how > > we display unsafe usages to the user; we can exclude variables from > > analysis entirely when all their unsafe uses are guarded by the pragma. I > > suspect that we can drop these gadgets as early as in `findGadgets()` phase > > (assuming that D140062 causes us to never rely on unsafe gadgets for > > fixits). > Let's addres this in a follow-up patch. I agree to both of you. Moving the check into `UnsafeBufferUsage.cpp` may depends on this https://reviews.llvm.org/D140062 patch. So I will make it a follow-up patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140179/new/ https://reviews.llvm.org/D140179 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits