NoQ added inline comments.
================
Comment at: docs/analyzer/checkers.rst:225-226
+``std::string``s, by recognizing member functions that may re/deallocate the
buffer
+before use. In the future, it would be great to add support for other STL and
+non-STL containers, and most notably, ``std::string_view``s.
+
----------------
Szelethus wrote:
> dkrupp wrote:
> > Szelethus wrote:
> > > Hmm. While this page is a documentation, I would still expect regular
> > > users to browse through it -- are we sure that we want to add future
> > > plans for a non-alpha checker? I'm not against it, just a question.
> > I think it is a good idea. A non-alpha checker can also be further
> > developed, by anyone else. It is good that we don't forget about further
> > features. This note also highlights the limitations of the checker.
> How about this: "Future plans include to add support for blahblah". The
> current statement should rather be a TODO in the code.
I suggest presenting it as "The checker is currently limited to `std::string`s
and doesn't recognize some of the more sophisticated approaches to passing
unowned pointers around, such as `std::string_view`s". It sounds a bit more
negative than it deserves to sound, but that's the most documentation-like text
i managed to come up with so far >.< Maybe put it under a "Known Limitations:"
marker and/or expand the main part of the documentation in order to keep the
reader's impression balanced, eg. "Many container methods in the C++ standard
library are known to invalidate "references" (including actual references,
iterators and raw pointers) to elements of the container. Using such references
after they are invalidated causes undefined behavior, which is a common source
of memory errors in C++ that this checker is capable of finding."
> While this page is a documentation, I would still expect regular users to
> browse through it
I'd love our users to browse it! Maybe we should consider adding a
documentation link to our HTML report headers as the documentation gets good
enough.
================
Comment at: docs/analyzer/checkers.rst:232
+
+ void test_deref_after_equals() {
+ std::string s = "llvm";
----------------
The `test_` part doesn't add much here, maybe drop it?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60281/new/
https://reviews.llvm.org/D60281
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
