[PATCH] D35520: [Sema] Improve diagnostic message for unavailable c++17 aligned allocation functions

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: include/clang/Basic/AlignedAllocation.h:25
+
+static inline VersionTuple alignedAllocMinVersion(llvm::Triple::OSType OS) {
+  switch (OS) {
----------------
Redundant `static`?


================
Comment at: include/clang/Basic/AlignedAllocation.h:31
+  case llvm::Triple::MacOSX: // Earliest supporting version is 10.13.
+    return VersionTuple(10U, 13U, 0U);
+  case llvm::Triple::IOS:
----------------
You can drop the last `0` here and two last zeros down below. This will also make the diagnostics less verbose, i.e. instead of `iOS 11.0.0` we will show `iOS 11`.


================
Comment at: include/clang/Basic/AlignedAllocation.h:42
+
+}
+
----------------
`// end namespace clang`


================
Comment at: include/clang/Basic/AlignedAllocation.h:44
+
+#endif
----------------
NIT: Missing `// LLVM_CLANG_BASIC_ALIGNED_ALLOCATION_H` comment


================
Comment at: lib/Sema/SemaExprCXX.cpp:1667
     S.Diag(Loc, diag::warn_aligned_allocation_unavailable)
-         << IsDelete << FD.getType().getAsString()
-         << S.getASTContext().getTargetInfo().getTriple().str();
+         << IsDelete << FD.getType().getAsString() << T.getOSTypeName(T.getOS())
+         << alignedAllocMinVersion(T.getOS()).getAsString();
----------------
Can you use `AvailabilityAttr::getPlatformNameSourceSpelling` instead to be consisted with our unguarded availability warnings?
e.g.:

```
AvailabilityAttr::getPlatformNameSourceSpelling(S.getASTContext().getTargetInfo().getPlatformName())
```

This will ensure that the diagnostics will use names like 'macOS' instead of 'macosx'.


https://reviews.llvm.org/D35520