- User Since
- Jul 19 2017, 6:59 AM (108 w, 4 d)
Thank you, and sorry for dragging you through this! At the very least, we got to learn a lot from it :)
Fri, Aug 16
I hope you dont mind me changing the revision title -- many of us are subscribed to the "analyzer" tag and like learning about whats changing :)
I really like the change, thanks! Let's settle on the diagnostic message then.
Thu, Aug 15
You seem to have diffed against your latest local commit, rather than against trunk (or master, if you use the monorepo). Phabricator isn't smart enough to put two and two together, and only displays the uploaded diff (though one has to admit, its doing a damn good job at that at least!).
Oh, there is no need for a new differential, you can update this one by clicking on 'update diff' in the right panel.
Yup, you can upload it and the green checkmark will stay. If it doesn't, I'll accept again.
Wed, Aug 14
Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.
Revised the documentation according to @NoQ's comments. By literally copy pasting it. Like any good programmer should do :^)
I'll gladly add the finishing touches :)
I really like the high level idea proposed by this patch, and the test files make me believe that its correct as well! I'm really not familiar around this part of the code, so if its okay, I'll take my time to do the usual find references, inserting unreachables/asserts to gain a better understanding before the green checkmark.
I agree with @gamesh411 here, would it be much trouble to see how this behaves on a bigger codebase?
I have no authority to accept patches in clang-tidy (though please feel free to add me as a reviewer, I can more easily participate in the discussion!), but in any case, this looks good to me too, thanks!
- Make the generic messages in-class, constexpr fields.
- Provide plenty of test cases for captured lambda variables. In our meeting we had both had a sight of relief, and some still lingering concerns about this, so maybe it deserves fixing in the long term.
You know what, I argued that we should use configs because this flag is incomplete. So I shouldn't be all up and down that it isn't.
I swear this is my last objection :) As soon as this is settled, I'll accept.
Now that I had ever more time to think about this patch, I see a great potential in it for development use, for example, we could silence a checker before splitting it up to see whether we could disable it altogether or really having to go through the process of splitting it into a modeling and reporting portion.
Thanks!!! Please note that BugReporter.cpp (especially the parts you touched) just got refactored extensively, so you'll need to rebase on master.
Hmm, why the need for checker options? Why not have them by default? If fixits are an experimental feature, maybe we should have a global enable-fixits config maybe. But I don't insist :)
Tue, Aug 13
This really shouldn't matter much. Go for it!
If you have no major objections, could you please accept formally? :)
LGTM, thanks! Do you need someone to commit this on your behalf? Also, could you please make the comments capitalized, terminated, and fitting in 80 columns?
The deed is done.
We might also want to change the revision title, but the commit message for sure, to make it clear that this affects all checkers, something along the lines of "[analyzer] Add an analyzer config to silence diagnostics from user specified checkers/packages"
Oh, crap, forgot to address inlines. I'll see how buildbots react to these changes and will follow up in a smaller patch. Reopening as a reminder.
Apologies for going off-topic, but would it make sense to start lifetime related patches with the tag [LifetimeAnalysis] or similar (like [analyzer] for static analyzer patches)? I dont feel knowledgable enough to participate, but would gladly add a herald rule to be subscribed and learn about the development process of it.