Use of unnecessary conditions checks Inside clang/lib/AST/ASTImporter.cpp

Subprojects LLDB
#1

I was looking into the codebase of clang/lib/AST/ASTImporter.cpp, at the line no. 5872

  if (DTemplated->isThisDeclarationADefinition() &&
      !ToTemplated->isThisDeclarationADefinition()) {
    // FIXME: Import definition!
  }

there is similar code at line no. 5589 also. I’m curious about what is the need of Import definition and this fixme is since 2013 don’t know why , It’s more like it is irrelevant and then checking the conditions doesn’t make sense, ultimately might be it’ll increase the time because of the irrelevant checking . If there is no solid need of these conditions , we should remove this ! Want to know more about it , Please comment your views.
CC: @shafik , @AaronBallman , @martong

#2

(Trying this reply out from email instead of the Discourse web interface. If something looks wrong to you, let me know!)

As best I can tell, those FIXMEs have been there since 2013 with: https://github.com/llvm/llvm-project/commit/39a1e507ff0bef4bd6b2fdbab4e38583d2679617. I’m not certain the circumstances under which conditions further work is needed here. I didn’t spot any test cases in that commit which related to PCH.

#3

Hi,

First of all, sorry for the late answer, took some time to get used to with discourse. Second, I think this is a good catch. In VisitVarTemplateDecl we have what you mention

  if (DTemplated->isThisDeclarationADefinition() &&
      !ToTemplated->isThisDeclarationADefinition()) {
    // FIXME: Import definition!
  }

And in VisitClassTemplateDecl the FIXME is slightly different, but does something similar.

  if (FromTemplated->isCompleteDefinition() &&
      !ToTemplated->isCompleteDefinition()) {
    // FIXME: Import definition!
  }

I do think that these conditions are unnecessary because in VisitClassTemplateDecl we import the definition via the templated CXXRecordDecl and in VisitVarTemplateDecl via the templated VarDecl. These are named ToTemplted and DTemplated respectively.

Please create a patch and make sure that the ASTTest unittests pass. E.g.

ninja ASTTests && ./tools/clang/unittests/AST/ASTTests

These lit tests should also pass

ninja check-clang-astmerge check-clang-import check-clang-analysis

Plus, check-lldb should not produce more failure with your patch than on the baseline of the patch.

Thanks!