OLD | NEW |
(Empty) | |
| 1 // Copyright (c) 2012 The Chromium Authors. All rights reserved. |
| 2 // Use of this source code is governed by a BSD-style license that can be |
| 3 // found in the LICENSE file. |
| 4 |
| 5 #include "FindBadConstructsConsumer.h" |
| 6 |
| 7 #include "clang/Frontend/CompilerInstance.h" |
| 8 #include "clang/AST/Attr.h" |
| 9 #include "clang/Lex/Lexer.h" |
| 10 #include "llvm/Support/raw_ostream.h" |
| 11 |
| 12 using namespace clang; |
| 13 |
| 14 namespace chrome_checker { |
| 15 |
| 16 namespace { |
| 17 |
| 18 const char kMethodRequiresOverride[] = |
| 19 "[chromium-style] Overriding method must be marked with 'override' or " |
| 20 "'final'."; |
| 21 const char kRedundantVirtualSpecifier[] = |
| 22 "[chromium-style] %0 is redundant; %1 implies %0."; |
| 23 // http://llvm.org/bugs/show_bug.cgi?id=21051 has been filed to make this a |
| 24 // Clang warning. |
| 25 const char kBaseMethodVirtualAndFinal[] = |
| 26 "[chromium-style] The virtual method does not override anything and is " |
| 27 "final; consider making it non-virtual."; |
| 28 const char kNoExplicitDtor[] = |
| 29 "[chromium-style] Classes that are ref-counted should have explicit " |
| 30 "destructors that are declared protected or private."; |
| 31 const char kPublicDtor[] = |
| 32 "[chromium-style] Classes that are ref-counted should have " |
| 33 "destructors that are declared protected or private."; |
| 34 const char kProtectedNonVirtualDtor[] = |
| 35 "[chromium-style] Classes that are ref-counted and have non-private " |
| 36 "destructors should declare their destructor virtual."; |
| 37 const char kWeakPtrFactoryOrder[] = |
| 38 "[chromium-style] WeakPtrFactory members which refer to their outer class " |
| 39 "must be the last member in the outer class definition."; |
| 40 const char kBadLastEnumValue[] = |
| 41 "[chromium-style] _LAST/Last constants of enum types must have the maximal " |
| 42 "value for any constant of that type."; |
| 43 const char kNoteInheritance[] = "[chromium-style] %0 inherits from %1 here"; |
| 44 const char kNoteImplicitDtor[] = |
| 45 "[chromium-style] No explicit destructor for %0 defined"; |
| 46 const char kNotePublicDtor[] = |
| 47 "[chromium-style] Public destructor declared here"; |
| 48 const char kNoteProtectedNonVirtualDtor[] = |
| 49 "[chromium-style] Protected non-virtual destructor declared here"; |
| 50 |
| 51 bool TypeHasNonTrivialDtor(const Type* type) { |
| 52 if (const CXXRecordDecl* cxx_r = type->getPointeeCXXRecordDecl()) |
| 53 return !cxx_r->hasTrivialDestructor(); |
| 54 |
| 55 return false; |
| 56 } |
| 57 |
| 58 // Returns the underlying Type for |type| by expanding typedefs and removing |
| 59 // any namespace qualifiers. This is similar to desugaring, except that for |
| 60 // ElaboratedTypes, desugar will unwrap too much. |
| 61 const Type* UnwrapType(const Type* type) { |
| 62 if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type)) |
| 63 return UnwrapType(elaborated->getNamedType().getTypePtr()); |
| 64 if (const TypedefType* typedefed = dyn_cast<TypedefType>(type)) |
| 65 return UnwrapType(typedefed->desugar().getTypePtr()); |
| 66 return type; |
| 67 } |
| 68 |
| 69 bool IsGtestTestFixture(const CXXRecordDecl* decl) { |
| 70 return decl->getQualifiedNameAsString() == "testing::Test"; |
| 71 } |
| 72 |
| 73 // Generates a fixit hint to remove the 'virtual' keyword. |
| 74 // Unfortunately, there doesn't seem to be a good way to determine the source |
| 75 // location of the 'virtual' keyword. It's available in Declarator, but that |
| 76 // isn't accessible from the AST. So instead, make an educated guess that the |
| 77 // first token is probably the virtual keyword. Strictly speaking, this doesn't |
| 78 // have to be true, but it probably will be. |
| 79 // TODO(dcheng): Add a warning to force virtual to always appear first ;-) |
| 80 FixItHint FixItRemovalForVirtual(const SourceManager& manager, |
| 81 const LangOptions& lang_opts, |
| 82 const CXXMethodDecl* method) { |
| 83 SourceRange range(method->getLocStart()); |
| 84 // Get the spelling loc just in case it was expanded from a macro. |
| 85 SourceRange spelling_range(manager.getSpellingLoc(range.getBegin())); |
| 86 // Sanity check that the text looks like virtual. |
| 87 StringRef text = clang::Lexer::getSourceText( |
| 88 CharSourceRange::getTokenRange(spelling_range), manager, lang_opts); |
| 89 if (text.trim() != "virtual") |
| 90 return FixItHint(); |
| 91 return FixItHint::CreateRemoval(range); |
| 92 } |
| 93 |
| 94 bool IsPodOrTemplateType(const CXXRecordDecl& record) { |
| 95 return record.isPOD() || |
| 96 record.getDescribedClassTemplate() || |
| 97 record.getTemplateSpecializationKind() || |
| 98 record.isDependentType(); |
| 99 } |
| 100 |
| 101 } // namespace |
| 102 |
| 103 FindBadConstructsConsumer::FindBadConstructsConsumer(CompilerInstance& instance, |
| 104 const Options& options) |
| 105 : ChromeClassTester(instance, options) { |
| 106 // Messages for virtual method specifiers. |
| 107 diag_method_requires_override_ = |
| 108 diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresOverride); |
| 109 diag_redundant_virtual_specifier_ = |
| 110 diagnostic().getCustomDiagID(getErrorLevel(), kRedundantVirtualSpecifier); |
| 111 diag_base_method_virtual_and_final_ = |
| 112 diagnostic().getCustomDiagID(getErrorLevel(), kBaseMethodVirtualAndFinal); |
| 113 |
| 114 // Messages for destructors. |
| 115 diag_no_explicit_dtor_ = |
| 116 diagnostic().getCustomDiagID(getErrorLevel(), kNoExplicitDtor); |
| 117 diag_public_dtor_ = |
| 118 diagnostic().getCustomDiagID(getErrorLevel(), kPublicDtor); |
| 119 diag_protected_non_virtual_dtor_ = |
| 120 diagnostic().getCustomDiagID(getErrorLevel(), kProtectedNonVirtualDtor); |
| 121 |
| 122 // Miscellaneous messages. |
| 123 diag_weak_ptr_factory_order_ = |
| 124 diagnostic().getCustomDiagID(getErrorLevel(), kWeakPtrFactoryOrder); |
| 125 diag_bad_enum_last_value_ = |
| 126 diagnostic().getCustomDiagID(getErrorLevel(), kBadLastEnumValue); |
| 127 |
| 128 // Registers notes to make it easier to interpret warnings. |
| 129 diag_note_inheritance_ = |
| 130 diagnostic().getCustomDiagID(DiagnosticsEngine::Note, kNoteInheritance); |
| 131 diag_note_implicit_dtor_ = |
| 132 diagnostic().getCustomDiagID(DiagnosticsEngine::Note, kNoteImplicitDtor); |
| 133 diag_note_public_dtor_ = |
| 134 diagnostic().getCustomDiagID(DiagnosticsEngine::Note, kNotePublicDtor); |
| 135 diag_note_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID( |
| 136 DiagnosticsEngine::Note, kNoteProtectedNonVirtualDtor); |
| 137 } |
| 138 |
| 139 bool FindBadConstructsConsumer::VisitDecl(clang::Decl* decl) { |
| 140 clang::TagDecl* tag_decl = dyn_cast<clang::TagDecl>(decl); |
| 141 if (tag_decl && tag_decl->isCompleteDefinition()) |
| 142 CheckTag(tag_decl); |
| 143 return true; |
| 144 } |
| 145 |
| 146 void FindBadConstructsConsumer::CheckChromeClass(SourceLocation record_location, |
| 147 CXXRecordDecl* record) { |
| 148 // By default, the clang checker doesn't check some types (templates, etc). |
| 149 // That was only a mistake; once Chromium code passes these checks, we should |
| 150 // remove the "check-templates" option and remove this code. |
| 151 // See crbug.com/441916 |
| 152 if (!options_.check_templates && IsPodOrTemplateType(*record)) |
| 153 return; |
| 154 |
| 155 bool implementation_file = InImplementationFile(record_location); |
| 156 |
| 157 if (!implementation_file) { |
| 158 // Only check for "heavy" constructors/destructors in header files; |
| 159 // within implementation files, there is no performance cost. |
| 160 |
| 161 // If this is a POD or a class template or a type dependent on a |
| 162 // templated class, assume there's no ctor/dtor/virtual method |
| 163 // optimization that we should do. |
| 164 if (!IsPodOrTemplateType(*record)) |
| 165 CheckCtorDtorWeight(record_location, record); |
| 166 } |
| 167 |
| 168 bool warn_on_inline_bodies = !implementation_file; |
| 169 // Check that all virtual methods are annotated with override or final. |
| 170 // Note this could also apply to templates, but for some reason Clang |
| 171 // does not always see the "override", so we get false positives. |
| 172 // See http://llvm.org/bugs/show_bug.cgi?id=18440 and |
| 173 // http://llvm.org/bugs/show_bug.cgi?id=21942 |
| 174 if (!IsPodOrTemplateType(*record)) |
| 175 CheckVirtualMethods(record_location, record, warn_on_inline_bodies); |
| 176 |
| 177 CheckRefCountedDtors(record_location, record); |
| 178 |
| 179 CheckWeakPtrFactoryMembers(record_location, record); |
| 180 } |
| 181 |
| 182 void FindBadConstructsConsumer::CheckChromeEnum(SourceLocation enum_location, |
| 183 EnumDecl* enum_decl) { |
| 184 if (!options_.check_enum_last_value) |
| 185 return; |
| 186 |
| 187 bool got_one = false; |
| 188 bool is_signed = false; |
| 189 llvm::APSInt max_so_far; |
| 190 EnumDecl::enumerator_iterator iter; |
| 191 for (iter = enum_decl->enumerator_begin(); |
| 192 iter != enum_decl->enumerator_end(); |
| 193 ++iter) { |
| 194 llvm::APSInt current_value = iter->getInitVal(); |
| 195 if (!got_one) { |
| 196 max_so_far = current_value; |
| 197 is_signed = current_value.isSigned(); |
| 198 got_one = true; |
| 199 } else { |
| 200 if (is_signed != current_value.isSigned()) { |
| 201 // This only happens in some cases when compiling C (not C++) files, |
| 202 // so it is OK to bail out here. |
| 203 return; |
| 204 } |
| 205 if (current_value > max_so_far) |
| 206 max_so_far = current_value; |
| 207 } |
| 208 } |
| 209 for (iter = enum_decl->enumerator_begin(); |
| 210 iter != enum_decl->enumerator_end(); |
| 211 ++iter) { |
| 212 std::string name = iter->getNameAsString(); |
| 213 if (((name.size() > 4 && name.compare(name.size() - 4, 4, "Last") == 0) || |
| 214 (name.size() > 5 && name.compare(name.size() - 5, 5, "_LAST") == 0)) && |
| 215 iter->getInitVal() < max_so_far) { |
| 216 diagnostic().Report(iter->getLocation(), diag_bad_enum_last_value_); |
| 217 } |
| 218 } |
| 219 } |
| 220 |
| 221 void FindBadConstructsConsumer::CheckCtorDtorWeight( |
| 222 SourceLocation record_location, |
| 223 CXXRecordDecl* record) { |
| 224 // We don't handle anonymous structs. If this record doesn't have a |
| 225 // name, it's of the form: |
| 226 // |
| 227 // struct { |
| 228 // ... |
| 229 // } name_; |
| 230 if (record->getIdentifier() == NULL) |
| 231 return; |
| 232 |
| 233 // Count the number of templated base classes as a feature of whether the |
| 234 // destructor can be inlined. |
| 235 int templated_base_classes = 0; |
| 236 for (CXXRecordDecl::base_class_const_iterator it = record->bases_begin(); |
| 237 it != record->bases_end(); |
| 238 ++it) { |
| 239 if (it->getTypeSourceInfo()->getTypeLoc().getTypeLocClass() == |
| 240 TypeLoc::TemplateSpecialization) { |
| 241 ++templated_base_classes; |
| 242 } |
| 243 } |
| 244 |
| 245 // Count the number of trivial and non-trivial member variables. |
| 246 int trivial_member = 0; |
| 247 int non_trivial_member = 0; |
| 248 int templated_non_trivial_member = 0; |
| 249 for (RecordDecl::field_iterator it = record->field_begin(); |
| 250 it != record->field_end(); |
| 251 ++it) { |
| 252 CountType(it->getType().getTypePtr(), |
| 253 &trivial_member, |
| 254 &non_trivial_member, |
| 255 &templated_non_trivial_member); |
| 256 } |
| 257 |
| 258 // Check to see if we need to ban inlined/synthesized constructors. Note |
| 259 // that the cutoffs here are kind of arbitrary. Scores over 10 break. |
| 260 int dtor_score = 0; |
| 261 // Deriving from a templated base class shouldn't be enough to trigger |
| 262 // the ctor warning, but if you do *anything* else, it should. |
| 263 // |
| 264 // TODO(erg): This is motivated by templated base classes that don't have |
| 265 // any data members. Somehow detect when templated base classes have data |
| 266 // members and treat them differently. |
| 267 dtor_score += templated_base_classes * 9; |
| 268 // Instantiating a template is an insta-hit. |
| 269 dtor_score += templated_non_trivial_member * 10; |
| 270 // The fourth normal class member should trigger the warning. |
| 271 dtor_score += non_trivial_member * 3; |
| 272 |
| 273 int ctor_score = dtor_score; |
| 274 // You should be able to have 9 ints before we warn you. |
| 275 ctor_score += trivial_member; |
| 276 |
| 277 if (ctor_score >= 10) { |
| 278 if (!record->hasUserDeclaredConstructor()) { |
| 279 emitWarning(record_location, |
| 280 "Complex class/struct needs an explicit out-of-line " |
| 281 "constructor."); |
| 282 } else { |
| 283 // Iterate across all the constructors in this file and yell if we |
| 284 // find one that tries to be inline. |
| 285 for (CXXRecordDecl::ctor_iterator it = record->ctor_begin(); |
| 286 it != record->ctor_end(); |
| 287 ++it) { |
| 288 // The current check is buggy. An implicit copy constructor does not |
| 289 // have an inline body, so this check never fires for classes with a |
| 290 // user-declared out-of-line constructor. |
| 291 if (it->hasInlineBody()) { |
| 292 if (it->isCopyConstructor() && |
| 293 !record->hasUserDeclaredCopyConstructor()) { |
| 294 // In general, implicit constructors are generated on demand. But |
| 295 // in the Windows component build, dllexport causes instantiation of |
| 296 // the copy constructor which means that this fires on many more |
| 297 // classes. For now, suppress this on dllexported classes. |
| 298 // (This does mean that windows component builds will not emit this |
| 299 // warning in some cases where it is emitted in other configs, but |
| 300 // that's the better tradeoff at this point). |
| 301 // TODO(dcheng): With the RecursiveASTVisitor, these warnings might |
| 302 // be emitted on other platforms too, reevaluate if we want to keep |
| 303 // surpressing this then http://crbug.com/467288 |
| 304 if (!record->hasAttr<DLLExportAttr>()) |
| 305 emitWarning(record_location, |
| 306 "Complex class/struct needs an explicit out-of-line " |
| 307 "copy constructor."); |
| 308 } else { |
| 309 // See the comment in the previous branch about copy constructors. |
| 310 // This does the same for implicit move constructors. |
| 311 bool is_likely_compiler_generated_dllexport_move_ctor = |
| 312 it->isMoveConstructor() && |
| 313 !record->hasUserDeclaredMoveConstructor() && |
| 314 record->hasAttr<DLLExportAttr>(); |
| 315 if (!is_likely_compiler_generated_dllexport_move_ctor) |
| 316 emitWarning(it->getInnerLocStart(), |
| 317 "Complex constructor has an inlined body."); |
| 318 } |
| 319 } else if (it->isInlined() && !it->isInlineSpecified() && |
| 320 !it->isDeleted() && (!it->isCopyOrMoveConstructor() || |
| 321 it->isExplicitlyDefaulted())) { |
| 322 // isInlined() is a more reliable check than hasInlineBody(), but |
| 323 // unfortunately, it results in warnings for implicit copy/move |
| 324 // constructors in the previously mentioned situation. To preserve |
| 325 // compatibility with existing Chromium code, only warn if it's an |
| 326 // explicitly defaulted copy or move constructor. |
| 327 emitWarning(it->getInnerLocStart(), |
| 328 "Complex constructor has an inlined body."); |
| 329 } |
| 330 } |
| 331 } |
| 332 } |
| 333 |
| 334 // The destructor side is equivalent except that we don't check for |
| 335 // trivial members; 20 ints don't need a destructor. |
| 336 if (dtor_score >= 10 && !record->hasTrivialDestructor()) { |
| 337 if (!record->hasUserDeclaredDestructor()) { |
| 338 emitWarning(record_location, |
| 339 "Complex class/struct needs an explicit out-of-line " |
| 340 "destructor."); |
| 341 } else if (CXXDestructorDecl* dtor = record->getDestructor()) { |
| 342 if (dtor->isInlined() && !dtor->isInlineSpecified() && |
| 343 !dtor->isDeleted()) { |
| 344 emitWarning(dtor->getInnerLocStart(), |
| 345 "Complex destructor has an inline body."); |
| 346 } |
| 347 } |
| 348 } |
| 349 } |
| 350 |
| 351 bool FindBadConstructsConsumer::InTestingNamespace(const Decl* record) { |
| 352 return GetNamespace(record).find("testing") != std::string::npos; |
| 353 } |
| 354 |
| 355 bool FindBadConstructsConsumer::IsMethodInBannedOrTestingNamespace( |
| 356 const CXXMethodDecl* method) { |
| 357 if (InBannedNamespace(method)) |
| 358 return true; |
| 359 for (CXXMethodDecl::method_iterator i = method->begin_overridden_methods(); |
| 360 i != method->end_overridden_methods(); |
| 361 ++i) { |
| 362 const CXXMethodDecl* overridden = *i; |
| 363 if (IsMethodInBannedOrTestingNamespace(overridden) || |
| 364 // Provide an exception for ::testing::Test. gtest itself uses some |
| 365 // magic to try to make sure SetUp()/TearDown() aren't capitalized |
| 366 // incorrectly, but having the plugin enforce override is also nice. |
| 367 (InTestingNamespace(overridden) && |
| 368 !IsGtestTestFixture(overridden->getParent()))) { |
| 369 return true; |
| 370 } |
| 371 } |
| 372 |
| 373 return false; |
| 374 } |
| 375 |
| 376 SuppressibleDiagnosticBuilder |
| 377 FindBadConstructsConsumer::ReportIfSpellingLocNotIgnored( |
| 378 SourceLocation loc, |
| 379 unsigned diagnostic_id) { |
| 380 return SuppressibleDiagnosticBuilder( |
| 381 &diagnostic(), loc, diagnostic_id, |
| 382 InBannedDirectory(instance().getSourceManager().getSpellingLoc(loc))); |
| 383 } |
| 384 |
| 385 // Checks that virtual methods are correctly annotated, and have no body in a |
| 386 // header file. |
| 387 void FindBadConstructsConsumer::CheckVirtualMethods( |
| 388 SourceLocation record_location, |
| 389 CXXRecordDecl* record, |
| 390 bool warn_on_inline_bodies) { |
| 391 // Gmock objects trigger these for each MOCK_BLAH() macro used. So we have a |
| 392 // trick to get around that. If a class has member variables whose types are |
| 393 // in the "testing" namespace (which is how gmock works behind the scenes), |
| 394 // there's a really high chance we won't care about these errors |
| 395 for (CXXRecordDecl::field_iterator it = record->field_begin(); |
| 396 it != record->field_end(); |
| 397 ++it) { |
| 398 CXXRecordDecl* record_type = it->getTypeSourceInfo() |
| 399 ->getTypeLoc() |
| 400 .getTypePtr() |
| 401 ->getAsCXXRecordDecl(); |
| 402 if (record_type) { |
| 403 if (InTestingNamespace(record_type)) { |
| 404 return; |
| 405 } |
| 406 } |
| 407 } |
| 408 |
| 409 for (CXXRecordDecl::method_iterator it = record->method_begin(); |
| 410 it != record->method_end(); |
| 411 ++it) { |
| 412 if (it->isCopyAssignmentOperator() || isa<CXXConstructorDecl>(*it)) { |
| 413 // Ignore constructors and assignment operators. |
| 414 } else if (isa<CXXDestructorDecl>(*it) && |
| 415 !record->hasUserDeclaredDestructor()) { |
| 416 // Ignore non-user-declared destructors. |
| 417 } else if (!it->isVirtual()) { |
| 418 continue; |
| 419 } else { |
| 420 CheckVirtualSpecifiers(*it); |
| 421 if (warn_on_inline_bodies) |
| 422 CheckVirtualBodies(*it); |
| 423 } |
| 424 } |
| 425 } |
| 426 |
| 427 // Makes sure that virtual methods use the most appropriate specifier. If a |
| 428 // virtual method overrides a method from a base class, only the override |
| 429 // specifier should be used. If the method should not be overridden by derived |
| 430 // classes, only the final specifier should be used. |
| 431 void FindBadConstructsConsumer::CheckVirtualSpecifiers( |
| 432 const CXXMethodDecl* method) { |
| 433 bool is_override = method->size_overridden_methods() > 0; |
| 434 bool has_virtual = method->isVirtualAsWritten(); |
| 435 OverrideAttr* override_attr = method->getAttr<OverrideAttr>(); |
| 436 FinalAttr* final_attr = method->getAttr<FinalAttr>(); |
| 437 |
| 438 if (IsMethodInBannedOrTestingNamespace(method)) |
| 439 return; |
| 440 |
| 441 SourceManager& manager = instance().getSourceManager(); |
| 442 const LangOptions& lang_opts = instance().getLangOpts(); |
| 443 |
| 444 // Complain if a method is annotated virtual && (override || final). |
| 445 if (has_virtual && (override_attr || final_attr)) { |
| 446 // ... but only if virtual does not originate in a macro from a banned file. |
| 447 // Note this is just an educated guess: the assumption here is that any |
| 448 // macro for declaring methods will probably be at the start of the method's |
| 449 // source range. |
| 450 ReportIfSpellingLocNotIgnored(method->getLocStart(), |
| 451 diag_redundant_virtual_specifier_) |
| 452 << "'virtual'" |
| 453 << (override_attr ? static_cast<Attr*>(override_attr) : final_attr) |
| 454 << FixItRemovalForVirtual(manager, lang_opts, method); |
| 455 } |
| 456 |
| 457 // Complain if a method is an override and is not annotated with override or |
| 458 // final. |
| 459 if (is_override && !override_attr && !final_attr) { |
| 460 SourceRange range = method->getSourceRange(); |
| 461 SourceLocation loc; |
| 462 if (method->hasInlineBody()) { |
| 463 loc = method->getBody()->getSourceRange().getBegin(); |
| 464 } else { |
| 465 loc = Lexer::getLocForEndOfToken(manager.getSpellingLoc(range.getEnd()), |
| 466 0, manager, lang_opts); |
| 467 // The original code used the ending source loc of TypeSourceInfo's |
| 468 // TypeLoc. Unfortunately, this breaks down in the presence of attributes. |
| 469 // Attributes often appear at the end of a TypeLoc, e.g. |
| 470 // virtual ULONG __stdcall AddRef() |
| 471 // has a TypeSourceInfo that looks something like: |
| 472 // ULONG AddRef() __attribute(stdcall) |
| 473 // so a fix-it insertion would be generated to insert 'override' after |
| 474 // __stdcall in the code as written. |
| 475 // While using the spelling loc of the CXXMethodDecl fixes attribute |
| 476 // handling, it breaks handling of "= 0" and similar constructs.. To work |
| 477 // around this, scan backwards in the source text for a '=' or ')' token |
| 478 // and adjust the location as needed... |
| 479 for (SourceLocation l = loc.getLocWithOffset(-1); |
| 480 l != manager.getLocForStartOfFile(manager.getFileID(loc)); |
| 481 l = l.getLocWithOffset(-1)) { |
| 482 l = Lexer::GetBeginningOfToken(l, manager, lang_opts); |
| 483 Token token; |
| 484 // getRawToken() returns *true* on failure. In that case, just give up |
| 485 // and don't bother generating a possibly incorrect fix-it. |
| 486 if (Lexer::getRawToken(l, token, manager, lang_opts, true)) { |
| 487 loc = SourceLocation(); |
| 488 break; |
| 489 } |
| 490 if (token.is(tok::r_paren)) { |
| 491 break; |
| 492 } else if (token.is(tok::equal)) { |
| 493 loc = l; |
| 494 break; |
| 495 } |
| 496 } |
| 497 } |
| 498 // Again, only emit the warning if it doesn't originate from a macro in |
| 499 // a system header. |
| 500 if (loc.isValid()) { |
| 501 ReportIfSpellingLocNotIgnored(loc, diag_method_requires_override_) |
| 502 << FixItHint::CreateInsertion(loc, " override"); |
| 503 } else { |
| 504 ReportIfSpellingLocNotIgnored(range.getBegin(), |
| 505 diag_method_requires_override_); |
| 506 } |
| 507 } |
| 508 |
| 509 if (final_attr && override_attr) { |
| 510 ReportIfSpellingLocNotIgnored(override_attr->getLocation(), |
| 511 diag_redundant_virtual_specifier_) |
| 512 << override_attr << final_attr |
| 513 << FixItHint::CreateRemoval(override_attr->getRange()); |
| 514 } |
| 515 |
| 516 if (final_attr && !is_override) { |
| 517 ReportIfSpellingLocNotIgnored(method->getLocStart(), |
| 518 diag_base_method_virtual_and_final_) |
| 519 << FixItRemovalForVirtual(manager, lang_opts, method) |
| 520 << FixItHint::CreateRemoval(final_attr->getRange()); |
| 521 } |
| 522 } |
| 523 |
| 524 void FindBadConstructsConsumer::CheckVirtualBodies( |
| 525 const CXXMethodDecl* method) { |
| 526 // Virtual methods should not have inline definitions beyond "{}". This |
| 527 // only matters for header files. |
| 528 if (method->hasBody() && method->hasInlineBody()) { |
| 529 if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) { |
| 530 if (cs->size()) { |
| 531 SourceLocation loc = cs->getLBracLoc(); |
| 532 // CR_BEGIN_MSG_MAP_EX and BEGIN_SAFE_MSG_MAP_EX try to be compatible |
| 533 // to BEGIN_MSG_MAP(_EX). So even though they are in chrome code, |
| 534 // we can't easily fix them, so explicitly whitelist them here. |
| 535 bool emit = true; |
| 536 if (loc.isMacroID()) { |
| 537 SourceManager& manager = instance().getSourceManager(); |
| 538 if (InBannedDirectory(manager.getSpellingLoc(loc))) |
| 539 emit = false; |
| 540 else { |
| 541 StringRef name = Lexer::getImmediateMacroName( |
| 542 loc, manager, instance().getLangOpts()); |
| 543 if (name == "CR_BEGIN_MSG_MAP_EX" || |
| 544 name == "BEGIN_SAFE_MSG_MAP_EX") |
| 545 emit = false; |
| 546 } |
| 547 } |
| 548 if (emit) |
| 549 emitWarning(loc, |
| 550 "virtual methods with non-empty bodies shouldn't be " |
| 551 "declared inline."); |
| 552 } |
| 553 } |
| 554 } |
| 555 } |
| 556 |
| 557 void FindBadConstructsConsumer::CountType(const Type* type, |
| 558 int* trivial_member, |
| 559 int* non_trivial_member, |
| 560 int* templated_non_trivial_member) { |
| 561 switch (type->getTypeClass()) { |
| 562 case Type::Record: { |
| 563 // Simplifying; the whole class isn't trivial if the dtor is, but |
| 564 // we use this as a signal about complexity. |
| 565 if (TypeHasNonTrivialDtor(type)) |
| 566 (*trivial_member)++; |
| 567 else |
| 568 (*non_trivial_member)++; |
| 569 break; |
| 570 } |
| 571 case Type::TemplateSpecialization: { |
| 572 TemplateName name = |
| 573 dyn_cast<TemplateSpecializationType>(type)->getTemplateName(); |
| 574 bool whitelisted_template = false; |
| 575 |
| 576 // HACK: I'm at a loss about how to get the syntax checker to get |
| 577 // whether a template is externed or not. For the first pass here, |
| 578 // just do retarded string comparisons. |
| 579 if (TemplateDecl* decl = name.getAsTemplateDecl()) { |
| 580 std::string base_name = decl->getNameAsString(); |
| 581 if (base_name == "basic_string") |
| 582 whitelisted_template = true; |
| 583 } |
| 584 |
| 585 if (whitelisted_template) |
| 586 (*non_trivial_member)++; |
| 587 else |
| 588 (*templated_non_trivial_member)++; |
| 589 break; |
| 590 } |
| 591 case Type::Elaborated: { |
| 592 CountType(dyn_cast<ElaboratedType>(type)->getNamedType().getTypePtr(), |
| 593 trivial_member, |
| 594 non_trivial_member, |
| 595 templated_non_trivial_member); |
| 596 break; |
| 597 } |
| 598 case Type::Typedef: { |
| 599 while (const TypedefType* TT = dyn_cast<TypedefType>(type)) { |
| 600 type = TT->getDecl()->getUnderlyingType().getTypePtr(); |
| 601 } |
| 602 CountType(type, |
| 603 trivial_member, |
| 604 non_trivial_member, |
| 605 templated_non_trivial_member); |
| 606 break; |
| 607 } |
| 608 default: { |
| 609 // Stupid assumption: anything we see that isn't the above is one of |
| 610 // the 20 integer types. |
| 611 (*trivial_member)++; |
| 612 break; |
| 613 } |
| 614 } |
| 615 } |
| 616 |
| 617 // Check |record| for issues that are problematic for ref-counted types. |
| 618 // Note that |record| may not be a ref-counted type, but a base class for |
| 619 // a type that is. |
| 620 // If there are issues, update |loc| with the SourceLocation of the issue |
| 621 // and returns appropriately, or returns None if there are no issues. |
| 622 // static |
| 623 FindBadConstructsConsumer::RefcountIssue |
| 624 FindBadConstructsConsumer::CheckRecordForRefcountIssue( |
| 625 const CXXRecordDecl* record, |
| 626 SourceLocation& loc) { |
| 627 if (!record->hasUserDeclaredDestructor()) { |
| 628 loc = record->getLocation(); |
| 629 return ImplicitDestructor; |
| 630 } |
| 631 |
| 632 if (CXXDestructorDecl* dtor = record->getDestructor()) { |
| 633 if (dtor->getAccess() == AS_public) { |
| 634 loc = dtor->getInnerLocStart(); |
| 635 return PublicDestructor; |
| 636 } |
| 637 } |
| 638 |
| 639 return None; |
| 640 } |
| 641 |
| 642 // Returns true if |base| specifies one of the Chromium reference counted |
| 643 // classes (base::RefCounted / base::RefCountedThreadSafe). |
| 644 bool FindBadConstructsConsumer::IsRefCounted( |
| 645 const CXXBaseSpecifier* base, |
| 646 CXXBasePath& path) { |
| 647 FindBadConstructsConsumer* self = this; |
| 648 const TemplateSpecializationType* base_type = |
| 649 dyn_cast<TemplateSpecializationType>( |
| 650 UnwrapType(base->getType().getTypePtr())); |
| 651 if (!base_type) { |
| 652 // Base-most definition is not a template, so this cannot derive from |
| 653 // base::RefCounted. However, it may still be possible to use with a |
| 654 // scoped_refptr<> and support ref-counting, so this is not a perfect |
| 655 // guarantee of safety. |
| 656 return false; |
| 657 } |
| 658 |
| 659 TemplateName name = base_type->getTemplateName(); |
| 660 if (TemplateDecl* decl = name.getAsTemplateDecl()) { |
| 661 std::string base_name = decl->getNameAsString(); |
| 662 |
| 663 // Check for both base::RefCounted and base::RefCountedThreadSafe. |
| 664 if (base_name.compare(0, 10, "RefCounted") == 0 && |
| 665 self->GetNamespace(decl) == "base") { |
| 666 return true; |
| 667 } |
| 668 } |
| 669 |
| 670 return false; |
| 671 } |
| 672 |
| 673 // Returns true if |base| specifies a class that has a public destructor, |
| 674 // either explicitly or implicitly. |
| 675 // static |
| 676 bool FindBadConstructsConsumer::HasPublicDtorCallback( |
| 677 const CXXBaseSpecifier* base, |
| 678 CXXBasePath& path, |
| 679 void* user_data) { |
| 680 // Only examine paths that have public inheritance, as they are the |
| 681 // only ones which will result in the destructor potentially being |
| 682 // exposed. This check is largely redundant, as Chromium code should be |
| 683 // exclusively using public inheritance. |
| 684 if (path.Access != AS_public) |
| 685 return false; |
| 686 |
| 687 CXXRecordDecl* record = |
| 688 dyn_cast<CXXRecordDecl>(base->getType()->getAs<RecordType>()->getDecl()); |
| 689 SourceLocation unused; |
| 690 return None != CheckRecordForRefcountIssue(record, unused); |
| 691 } |
| 692 |
| 693 // Outputs a C++ inheritance chain as a diagnostic aid. |
| 694 void FindBadConstructsConsumer::PrintInheritanceChain(const CXXBasePath& path) { |
| 695 for (CXXBasePath::const_iterator it = path.begin(); it != path.end(); ++it) { |
| 696 diagnostic().Report(it->Base->getLocStart(), diag_note_inheritance_) |
| 697 << it->Class << it->Base->getType(); |
| 698 } |
| 699 } |
| 700 |
| 701 unsigned FindBadConstructsConsumer::DiagnosticForIssue(RefcountIssue issue) { |
| 702 switch (issue) { |
| 703 case ImplicitDestructor: |
| 704 return diag_no_explicit_dtor_; |
| 705 case PublicDestructor: |
| 706 return diag_public_dtor_; |
| 707 case None: |
| 708 assert(false && "Do not call DiagnosticForIssue with issue None"); |
| 709 return 0; |
| 710 } |
| 711 assert(false); |
| 712 return 0; |
| 713 } |
| 714 |
| 715 // Check |record| to determine if it has any problematic refcounting |
| 716 // issues and, if so, print them as warnings/errors based on the current |
| 717 // value of getErrorLevel(). |
| 718 // |
| 719 // If |record| is a C++ class, and if it inherits from one of the Chromium |
| 720 // ref-counting classes (base::RefCounted / base::RefCountedThreadSafe), |
| 721 // ensure that there are no public destructors in the class hierarchy. This |
| 722 // is to guard against accidentally stack-allocating a RefCounted class or |
| 723 // sticking it in a non-ref-counted container (like scoped_ptr<>). |
| 724 void FindBadConstructsConsumer::CheckRefCountedDtors( |
| 725 SourceLocation record_location, |
| 726 CXXRecordDecl* record) { |
| 727 // Skip anonymous structs. |
| 728 if (record->getIdentifier() == NULL) |
| 729 return; |
| 730 |
| 731 // Determine if the current type is even ref-counted. |
| 732 CXXBasePaths refcounted_path; |
| 733 if (!record->lookupInBases( |
| 734 [this](const CXXBaseSpecifier* base, CXXBasePath& path) { |
| 735 return IsRefCounted(base, path); |
| 736 }, |
| 737 refcounted_path)) { |
| 738 return; // Class does not derive from a ref-counted base class. |
| 739 } |
| 740 |
| 741 // Easy check: Check to see if the current type is problematic. |
| 742 SourceLocation loc; |
| 743 RefcountIssue issue = CheckRecordForRefcountIssue(record, loc); |
| 744 if (issue != None) { |
| 745 diagnostic().Report(loc, DiagnosticForIssue(issue)); |
| 746 PrintInheritanceChain(refcounted_path.front()); |
| 747 return; |
| 748 } |
| 749 if (CXXDestructorDecl* dtor = |
| 750 refcounted_path.begin()->back().Class->getDestructor()) { |
| 751 if (dtor->getAccess() == AS_protected && !dtor->isVirtual()) { |
| 752 loc = dtor->getInnerLocStart(); |
| 753 diagnostic().Report(loc, diag_protected_non_virtual_dtor_); |
| 754 return; |
| 755 } |
| 756 } |
| 757 |
| 758 // Long check: Check all possible base classes for problematic |
| 759 // destructors. This checks for situations involving multiple |
| 760 // inheritance, where the ref-counted class may be implementing an |
| 761 // interface that has a public or implicit destructor. |
| 762 // |
| 763 // struct SomeInterface { |
| 764 // virtual void DoFoo(); |
| 765 // }; |
| 766 // |
| 767 // struct RefCountedInterface |
| 768 // : public base::RefCounted<RefCountedInterface>, |
| 769 // public SomeInterface { |
| 770 // private: |
| 771 // friend class base::Refcounted<RefCountedInterface>; |
| 772 // virtual ~RefCountedInterface() {} |
| 773 // }; |
| 774 // |
| 775 // While RefCountedInterface is "safe", in that its destructor is |
| 776 // private, it's possible to do the following "unsafe" code: |
| 777 // scoped_refptr<RefCountedInterface> some_class( |
| 778 // new RefCountedInterface); |
| 779 // // Calls SomeInterface::~SomeInterface(), which is unsafe. |
| 780 // delete static_cast<SomeInterface*>(some_class.get()); |
| 781 if (!options_.check_base_classes) |
| 782 return; |
| 783 |
| 784 // Find all public destructors. This will record the class hierarchy |
| 785 // that leads to the public destructor in |dtor_paths|. |
| 786 CXXBasePaths dtor_paths; |
| 787 if (!record->lookupInBases( |
| 788 [](const CXXBaseSpecifier* base, CXXBasePath& path) { |
| 789 // TODO(thakis): Inline HasPublicDtorCallback() after clang roll. |
| 790 return HasPublicDtorCallback(base, path, nullptr); |
| 791 }, |
| 792 dtor_paths)) { |
| 793 return; |
| 794 } |
| 795 |
| 796 for (CXXBasePaths::const_paths_iterator it = dtor_paths.begin(); |
| 797 it != dtor_paths.end(); |
| 798 ++it) { |
| 799 // The record with the problem will always be the last record |
| 800 // in the path, since it is the record that stopped the search. |
| 801 const CXXRecordDecl* problem_record = dyn_cast<CXXRecordDecl>( |
| 802 it->back().Base->getType()->getAs<RecordType>()->getDecl()); |
| 803 |
| 804 issue = CheckRecordForRefcountIssue(problem_record, loc); |
| 805 |
| 806 if (issue == ImplicitDestructor) { |
| 807 diagnostic().Report(record_location, diag_no_explicit_dtor_); |
| 808 PrintInheritanceChain(refcounted_path.front()); |
| 809 diagnostic().Report(loc, diag_note_implicit_dtor_) << problem_record; |
| 810 PrintInheritanceChain(*it); |
| 811 } else if (issue == PublicDestructor) { |
| 812 diagnostic().Report(record_location, diag_public_dtor_); |
| 813 PrintInheritanceChain(refcounted_path.front()); |
| 814 diagnostic().Report(loc, diag_note_public_dtor_); |
| 815 PrintInheritanceChain(*it); |
| 816 } |
| 817 } |
| 818 } |
| 819 |
| 820 // Check for any problems with WeakPtrFactory class members. This currently |
| 821 // only checks that any WeakPtrFactory<T> member of T appears as the last |
| 822 // data member in T. We could consider checking for bad uses of |
| 823 // WeakPtrFactory to refer to other data members, but that would require |
| 824 // looking at the initializer list in constructors to see what the factory |
| 825 // points to. |
| 826 // Note, if we later add other unrelated checks of data members, we should |
| 827 // consider collapsing them in to one loop to avoid iterating over the data |
| 828 // members more than once. |
| 829 void FindBadConstructsConsumer::CheckWeakPtrFactoryMembers( |
| 830 SourceLocation record_location, |
| 831 CXXRecordDecl* record) { |
| 832 // Skip anonymous structs. |
| 833 if (record->getIdentifier() == NULL) |
| 834 return; |
| 835 |
| 836 // Iterate through members of the class. |
| 837 RecordDecl::field_iterator iter(record->field_begin()), |
| 838 the_end(record->field_end()); |
| 839 SourceLocation weak_ptr_factory_location; // Invalid initially. |
| 840 for (; iter != the_end; ++iter) { |
| 841 const TemplateSpecializationType* template_spec_type = |
| 842 iter->getType().getTypePtr()->getAs<TemplateSpecializationType>(); |
| 843 bool param_is_weak_ptr_factory_to_self = false; |
| 844 if (template_spec_type) { |
| 845 const TemplateDecl* template_decl = |
| 846 template_spec_type->getTemplateName().getAsTemplateDecl(); |
| 847 if (template_decl && template_spec_type->getNumArgs() == 1) { |
| 848 if (template_decl->getNameAsString().compare("WeakPtrFactory") == 0 && |
| 849 GetNamespace(template_decl) == "base") { |
| 850 // Only consider WeakPtrFactory members which are specialized for the |
| 851 // owning class. |
| 852 const TemplateArgument& arg = template_spec_type->getArg(0); |
| 853 if (arg.getAsType().getTypePtr()->getAsCXXRecordDecl() == |
| 854 record->getTypeForDecl()->getAsCXXRecordDecl()) { |
| 855 if (!weak_ptr_factory_location.isValid()) { |
| 856 // Save the first matching WeakPtrFactory member for the |
| 857 // diagnostic. |
| 858 weak_ptr_factory_location = iter->getLocation(); |
| 859 } |
| 860 param_is_weak_ptr_factory_to_self = true; |
| 861 } |
| 862 } |
| 863 } |
| 864 } |
| 865 // If we've already seen a WeakPtrFactory<OwningType> and this param is not |
| 866 // one of those, it means there is at least one member after a factory. |
| 867 if (weak_ptr_factory_location.isValid() && |
| 868 !param_is_weak_ptr_factory_to_self) { |
| 869 diagnostic().Report(weak_ptr_factory_location, |
| 870 diag_weak_ptr_factory_order_); |
| 871 } |
| 872 } |
| 873 } |
| 874 |
| 875 } // namespace chrome_checker |
OLD | NEW |