Ericsson / clang

Cross Translation Unit analysis capability for Clang Static Analyzer. (Fork of official clang at http://llvm.org/git/clang)
http://clang.llvm.org/
Other
15 stars 10 forks source link

Call HandleNameConflict whenever there is a strucural inequivalence #589

Closed martong closed 5 years ago

martong commented 5 years ago

There are cases (FieldDecl) when we don't call HandleNameConflict but we should.

martong commented 5 years ago

654 fixes

martong commented 5 years ago

There is an other case where we should call HandleNameConflict in VisitClassTemplateSpecializationDecl:

    if (isStructuralMatch(D, PrevDecl)) {
      if (D->isThisDeclarationADefinition() && PrevDecl->getDefinition()) {
        Importer.MapImported(D, PrevDecl->getDefinition());
        // Import those those default field initializers which have been
        // instantiated in the "From" context, but not in the "To" context.
        for (auto *FromField : D->fields()) {
          auto ToOrErr = import(FromField);
          if (!ToOrErr)
            // FIXME: return the error?
            consumeError(ToOrErr.takeError());
        }

        // Import those methods which have been instantiated in the
        // "From" context, but not in the "To" context.
        for (CXXMethodDecl *FromM : D->methods()) {
          auto ToOrErr = import(FromM);
          if (!ToOrErr)
            // FIXME: return the error?
            consumeError(ToOrErr.takeError());
        }

        // TODO Import instantiated default arguments.
        // TODO Import instantiated exception specifications.
        //
        // Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
        // what
        // else could be fused during an AST merge.
        return PrevDecl;
      }
    } else { // ODR violation.
// HERE WE SHOULD CALL HandleNameConflict!
      return make_error<ImportError>(ImportError::NameConflict);
    }

There are open questions here:

balazske commented 5 years ago

I think handleNameConflict should not be called in this case. This is an ODR conflict but the problem is not correctable with a simple rename (or the template must be renamed somehow). It can be possible to use the ODR handling strategy 'liberal' if the list of template specializations (or other things in the AST) allows it that for the same parameter set multiple specializations are added.

martong commented 5 years ago

Okay, that is a good point. I don't think we could add multiple specializations for the same template arguments. This means neither the renaming nor the "liberal" strategy could work. So in this case only our "conservative" strategy can work, but that is how it is already implemented.