@MaskRay Very much appreciate your reminder! I missed Aaron's comments.

• Fix the clang/test/Misc/warning-flags.c failure pointed out by Aaron
• Move the test file to clang/Frontend as MaskRay suggested.

Appreciate your comments!

Improve the diagnostic message and trim the spaces around the feature flags.

Sorry for the delayed response. Adding the regression test is not as simple as I thought, first of all clang-tools-extra does not enable the assert, see https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/lit.site.cfg.py.in . And add the clang-doc when enable --format=html is little bit of tricky.

In D123924#3482342, @aaron.ballman wrote:

I don't know enough about this code base to feel comfortable signing off on it, but the changes look reasonable to me FWIW.

Appreciate your fix, LGTM. I didn't notice the subtle differences between the option prefixes. Please wait for the last review from @MaskRay before landing it.

I appreciate your guidance and the feedback, thanks : )

Sorry for the patch abuse, the pre-merge build bot didn't catch the failures, like https://lab.llvm.org/buildbot/#/builders/124/builds/3948 and https://lab.llvm.org/buildbot/#/builders/67/builds/6214. Hopefully, this patch will fix it.

In D121809#3388356, @tejohnson wrote:

In D121809#3388287, @MTC wrote:

Sorry to interrupt, seems this patch caused the build failure, see https://lab.llvm.org/buildbot/#/builders/124/builds/3944. I don't have any clue why this failed, Did I miss something for the test file? Like ; REQUIRES: x86 is indispensable?

Ah, for stats printing I believe you need a "REQUIRES: asserts"

Sorry to interrupt, seems this patch caused the build failure, see https://lab.llvm.org/buildbot/#/builders/124/builds/3944. I don't have any clue why this failed, Did I miss something for the test file? Like ; REQUIRES: x86 is indispensable?

Thanks for all the kind comments, I have updated the patch.

NFC, improve the comments and the test.

FYI: I'm from Bytedance Inc, @Sockke, and I are fixing the AutoFix bugs recently, most of them will lead to the compilation error. For this bug, fixing the override virtual methods but missing the pure virtual base method, which will cause the compilation error. These annoying bugs will hinder the large-scale deployment of clang-tidy AutoFix in the production environment.

This patch has become very complicated now. I summarized this patch and give a figure to illustrate what we have reached. And @Sockke please add some comments to explain the complex part or other means to make this patch more readable.

Hi @NoQ, sorry to bother you. This patch has been on hold for a very long time. It should not have been fixed yet. What's next? Abandon the patch, or improve it yourself?

In D108019#2945953, @MaskRay wrote:

Perhaps switch to no-demangle by default and add --symbolize-demangle?

I think we're a bit off track, and @Sockke wants to accomplish more than one goal in the same patch. I have summarized what we are currently discussing as follow shows:

Thanks for the cleanup.

In D106431#2892859, @whisperity wrote:

Is this the right decision to make, conceptually? It will leave the variable uninitialised still, and reading such an uninit variable is still an issue, even if it is an enum.

Yeah, that's right. However, it's much more difficult to give enum an initial value than an integer.

LGTM, thanks for your cleanup.

Good catch, the fix is reasonable to me. However, I'm no longer wok on the analyzer now, maybe you can add @NoQ Or some other active reviewers to review the code.

In D55388#1322601, @xazax.hun wrote:

Hm. I wonder if it would also make sense to model e.g. the get method to return nullptr for moved from smart ptrs. This could help null dereference checker and also aid false path prunning.

Please add more context using the -U option, like git diff -U99999 ....

In D54878#1307726, @ilya-biryukov wrote:

But given that isa<> are still better than dyn_cast<>, this change might still be worth landing.

We can land this change this time or do the cleaning job in other patches in the future, it's all up to you guys, the active clangd contributors :).

Use more concise form.

Thank you for doing the cleaning that no one else is interested in! Comments is inline below.

The "moved-from" terminology we adopt here still feels a bit weird to me, but i don't have a better suggestion, so i just removed the single-quotes so that to at least feel proud about what we have.

I am personally fine with this terminology, this checker corresponds to the cert rule EXP63-CPP. Do not rely on the value of a moved-from object, and moved from is also used in many places in CppCoreGuidelines.

I'm totally fine with this patch personally. However I am not familiar with this part, so can't give substantial help :).

Sorry for the long delay for this patch! The implementation is fine for me. However, I'm the newbie on clang diagnostics and have no further insight into this checker. @aaron.ballman may have more valuable insights into this checker.

In addition, clang/lib/StaticAnalyzer/README.txt and other related docs in clang/lib/docs/analyzer are also out of date.

In D52949#1268640, @xbolva00 wrote:

Second thought, I don't think we should recommend std::size here (maybe it should be okay for clang static analyzers)

uint32_t data[] = {10, 20, 30, 40};
len = sizeof(data)/sizeof(*data); // "warn" on valid code to recommend std::size? I dont agree with such behaviour.

In D53024#1258976, @Szelethus wrote:

Also, a lot of items on this list is outdated, but I joined the project relatively recently, so I'm not sure what's the state on all of them.

Greate idea! If we can enrich this list, it will not only help the reviewer, but also help beginners, like me, avoid some pitfalls when developing a new checker.

Thank you for digging in to delete that meaningless constructor, NoQ! And sorry for my technology debt : ).

In D48027#1209844, @NoQ wrote:

So i believe that one of the important remaining problems with CallDescription is to teach it to discriminate between global functions and methods. We can do it in a number of ways:

1. Make a special sub-class for CallDescription depending on the sort of the function (too verbose),
2. Tag all items in the list as "record" vs. "namespace" (too many tags),
3. Dunno, tons of other boring and verbose approaches,
4. Change our PreCall/PostCall callbacks to callback templates that let allow the user subscribe to specific sub-classes of CallEvent similarly to how he can subscribe to different kinds of statements in PreStmt<Class>/PostStmt<Class>, and then the user doesn't need to write any code to check it dynamically.

In D48027#1207645, @xazax.hun wrote:

Sorry for the delay, I think this is OK to commit.
As a possible improvement, I can imagine making it slightly stricter, e.g. you could only skip QualifiedNames for inline namespaces and the beginning. Maybe add support for staring with "" or :: to even disable skipping namespaces at the beginning?
But these are just nice to have features, I am perfectly ok with not having them or doing it in a followup patch.

@xazax.hun What do you think?

In D48027#1201248, @xazax.hun wrote:

Generally looks good, I only wonder if this works well with inline namespaces. Could you test? Probably it will.

• rebase
• Since we have enhanced the ability of CallDescription, remove the helper method isCalledOnStringObject().

kindly ping!

@xazax.hun Thanks for your tips! After some investigation, MatchFinder::match just traverse one ASTNode, that means match(namedDecl(HasNameMatcher())) and match(namedDecl(matchesName())) both not traverse children. So there are three ways to match the specified AST node.

Thanks for your review, NoQ!

kindly ping!