[PATCH] D90299: [windows-itanium] handle dllimport/export code paths separately and share with PS4

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:6513
   bool IsInline = false, IsStaticDataMember = false, IsQualifiedFriend = false;
-  bool IsMicrosoft =
-      S.Context.getTargetInfo().getCXXABI().isMicrosoft() ||
-      S.Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment();
+  bool IsMicrosoft = S.Context.getTargetInfo().shouldDLLImportComdatSymbols();
   if (const auto *VD = dyn_cast<VarDecl>(NewDecl)) {
----------------
bd1976llvm wrote:
> compnerd wrote:
> > I think that renaming `IsMicrosoft` is now warranted.
> What would be better though? The MS ABI is the standard for the dllimport/export behaviour. Therefore, I think IsMicrosoft  is quite a good name and it fits well with the existing comments.
Then `IsMicrosoftABI` sounds better :). The problem with the current name is that its confusing between the Microsoft environment and the Microsoft ABI.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:9766
         // attribute to a template with a previous instantiation declaration.
         // MinGW doesn't allow this.
         auto *A = cast<InheritableAttr>(
----------------
bd1976llvm wrote:
> compnerd wrote:
> > Please update the comment (I think we can entirely remove it now given the new name).  This probably fits better in the definition of `shouldImportDLLImportComdatSymbols` now as the condition is completely different now.
> The comment seems helpful to me though, no? Also, this comment is rather tied to the matching comment on L9782. Since the MS ABI is standard for dllimport/export behaviour and MinGW is the outlier (in that it does it's own thing, a bit, as far as C++ dllimport/export goes) it seems reasonable that these two are mentioned?
I think I would be okay with retaining the comment with the initial clause `In the MS ABI, ` removed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90299/new/

https://reviews.llvm.org/D90299