On Feb 23, 2011, at 9:23 AM, Chandler Carruth wrote:
> One overall concern I would have is that a parser struct is quadrupling in
> size. Can you measure the peak memory usage impact of this change on a hefty
> compilation?
It shouldn't have any effect whatsoever, since PointerTypeInfo is only used
when it's in a union with the much-larger FunctionTypeInfo.
- Doug
> Some nits below...
>
> Index: test/SemaCXX/return.cpp
> ===================================================================
> --- test/SemaCXX/return.cpp (revision 126218)
> +++ test/SemaCXX/return.cpp (working copy)
> @@ -24,5 +24,14 @@
> const volatile S class_cv();
>
> const int scalar_c(); // expected-warning{{'const' type qualifier on return
> type has no effect}}
> +
> +const
> +char*
> +const // expected-warning{{'const' type qualifier on return type has no
> effect}}
> +f();
>
> It would be good to test "T const" and "T const * const" as well for
> completeness...
>
> Index: include/clang/Sema/DeclSpec.h
> ===================================================================
> --- include/clang/Sema/DeclSpec.h (revision 126218)
> +++ include/clang/Sema/DeclSpec.h (working copy)
> @@ -825,6 +825,25 @@
> struct PointerTypeInfo : TypeInfoCommon {
> /// The type qualifiers: const/volatile/restrict.
> unsigned TypeQuals : 3;
> +
> + /// The location of the const-qualifier, if any.
> + unsigned ConstQualLoc;
> +
> + /// The location of the volatile-qualifier, if any.
> + unsigned VolatileQualLoc;
> +
> + /// The location of the restrict-qualifier, if any.
> + unsigned RestrictQualLoc;
> +
> + SourceLocation constQualLoc() const {
> + return SourceLocation::getFromRawEncoding(ConstQualLoc);
> + }
> + SourceLocation volatileQualLoc() const {
> + return SourceLocation::getFromRawEncoding(VolatileQualLoc);
> + }
> + SourceLocation restrictQualLoc() const {
> + return SourceLocation::getFromRawEncoding(RestrictQualLoc);
> + }
>
> I find the getters here a bit odd. As these are only accessed on one place, I
> wonder if we should do the raw encoding -> SourceLocation transition there.
>
> Perhaps most odd to me is that the members are public and doxymented, but
> they have non-trivial getters and those aren't doxymented. If you want
> getters on this struct, I'd make the members private and move the comments.
>
> Index: lib/Sema/SemaType.cpp
> ===================================================================
> --- lib/Sema/SemaType.cpp (revision 126218)
> +++ lib/Sema/SemaType.cpp (working copy)
> @@ -1395,6 +1395,49 @@
> return QT;
> }
>
> +static void DiagnoseIgnoredQualifiers(unsigned Quals,
> + SourceLocation constQualLoc,
> + SourceLocation volatileQualLoc,
> + SourceLocation restrictQualLoc,
> + Sema& S) {
> + std::string QualStr;
> + unsigned NumQuals = 0;
> + SourceLocation Loc;
> +
> + if (Quals & Qualifiers::Const) {
> + Loc = constQualLoc;
> + ++NumQuals;
> + QualStr = "const";
> + }
> + if (Quals & Qualifiers::Volatile) {
> + if (NumQuals == 0) {
> + Loc = volatileQualLoc;
> + QualStr = "volatile";
> + } else
> + QualStr += " volatile";
>
> I think this section is moved verbatim, but while we're here, can we have {}s
> around this else block? Especially followed by a preincrement, I had to read
> in 3 times...
>
> + ++NumQuals;
> + }
> + if (Quals & Qualifiers::Restrict) {
> + if (NumQuals == 0) {
> + Loc = restrictQualLoc;
> + QualStr = "restrict";
> + } else
> + QualStr += " restrict";
>
> Same here.
>
> + ++NumQuals;
> + }
> +
> + assert(NumQuals > 0 && "No known qualifiers?");
> + Sema::SemaDiagnosticBuilder DB = S.Diag(Loc, diag::warn_qual_return_type);
> +
> + DB << QualStr << NumQuals;
> + if (Quals & Qualifiers::Const)
> + DB << FixItHint::CreateRemoval(constQualLoc);
> + if (Quals & Qualifiers::Volatile)
> + DB << FixItHint::CreateRemoval(volatileQualLoc);
> + if (Quals & Qualifiers::Restrict)
> + DB << FixItHint::CreateRemoval(restrictQualLoc);
>
> And I think we simplify this by having three empty FixItHint variable, and
> initializing them in each branch where we do the original checking. That
> should also avoid the need to store the diagnostic builder, and we can just
> stream the lot of this in directly.
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
