[cfe-commits] PATCH: Add support for C++ namespace-aware typo correcting

[Kaelyn Uhrain]

I just finished up a new version of my patch earlier this week, which no
longer uses the RecursiveASTVisitor (per chandlerc's suggestion).  I've
rebased the patch against the current SVN and have attached it for you to
test.  With the new version only the FixIt/typo.cpp and
SemaObjC/synth-provisional-ivars.m tests are failing, and for those two I'm
not entirely sure which parts of the behavior are correct/acceptable and
which might be a bug in my changes.

Cheers,
Kaelyn

On Tue, Mar 8, 2011 at 7:46 PM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Feb 22, 2011, at 11:21 AM, Kaelyn Uhrain wrote:
>
> > This patch implements checking available C++ namespaces when trying to
> find typo corrections for unknown identifiers.  There are currently 4 broken
> unit tests (CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp,
> CXX/class/class.local/p1.cpp, FixIt/typo.cpp, and
> SemaObjC/synth-provisional-ivars.m) which I'm fairly sure are cases where
> the tests need to be updated to reflect the new behavior instead of being
> indicative of actual code breakage, but I'm not 100% certain.  The new unit
> test, test/Sema/missing-namespace-qualifier-typo-corrections.cpp, is a good
> demonstration of the improved suggestions provided by this patch as most of
> the errors found by "clang -fsyntax-only" lack suggestions without this
> patch..
>
> This is really cool, and is be a great step forward in usability.
>
> My only really concern is about performance, so I did a quick measurement.
> My test was to put all of the C++ Standard Library headers into a PCH file,
> then include it in a program that amounted to, basically,
>
>        int main() {
>                vector<int> v;
>                std::Vector<int> v2;
>        }
>
> so there are two typo corrections. And here is some timing data from
> before/after the patch:
>
> Before:
>
> *** AST File Statistics:
>  18 stat cache hits
>  2 stat cache misses
>  97/39174 source location entries read (0.247613%)
>  11142/18925 types read (58.874504%)
>  16712/32353 declarations read (51.655178%)
>  1734/6785 identifiers read (25.556374%)
>  1861/70867 statements read (2.626046%)
>  0/1766 macros read (0.000000%)
>  188/3464 lexical declcontexts read (5.427252%)
>  43/695 visible declcontexts read (6.187050%)
>
> Wall time: 0m0.051s
>
> After:
>
> *** AST File Statistics:
>  18 stat cache hits
>  2 stat cache misses
>  97/39174 source location entries read (0.247613%)
>  17162/18925 types read (90.684280%)
>  28421/32353 declarations read (87.846565%)
>  3943/6785 identifiers read (58.113487%)
>  70371/70867 statements read (99.300095%)
>  0/1766 macros read (0.000000%)
>  3383/3464 lexical declcontexts read (97.661659%)
>  6080/695 visible declcontexts read (874.820129%)
>
> Wall time: 0m0.115s
>
> So it's a little over 2x slower, and (more importantly) deserializes a
> whole lot more from the PCH file. That's actually a concern, since typo
> correction still needs to be fast for many applications, and the additional
> disk I/O really becomes a problem as the PCH files get larger (e.g.,
> containing significant chunks of Boost).
>
> I have a recommendation. Rather than using the recursive AST visitor to
> walk the entire AST, can we instead
>
>  (1) First do the "identifier" lookup that we already do, to find the best
> name that we know about (which could be anywhere in the translation unit),
> then
>  (2) If lookup for that identifier fails, go look in all of the namespaces
> that we know about and see if the identifier is located in one of those.
>
> We'd have to have some list of known namespaces stored somewhere (and saved
> in the PCH file for fast retrieval), but at least this way we'd be paying
> the cost we already pay now to look at the identifiers + a cost that's
> O(number of unique namespaces).
>
> Does that seem reasonable?
>
>
>        - Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110309/df8e52fc/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-namespace-aware-typo-corrections.diff
Type: text/x-patch
Size: 27063 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110309/df8e52fc/attachment.bin>