[PATCH] D71082: Allow system header to provide their own implementation of some builtin

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Just a few more nits, and this LGTM. Thanks again!

IDK if Eli wanted to have another look at this; please wait until tomorrow before landing to give him time to comment.



================
Comment at: clang/lib/AST/Decl.cpp:3006
 
+bool FunctionDecl::isInlineBuiltin() const {
+  if (!getBuiltinID())
----------------
nit: If we're going with this naming, I think it's important to highlight that we're querying if this is a definition. So `isInlineBuiltinDefinition()`?


================
Comment at: clang/test/CodeGen/memcpy-nobuiltin.c:5
+//
+// CHECK-WITH-DECL: 'memcpy' will always overflow; destination buffer has size 1, but size argument is 2
+// CHECK-WITH-DECL-NOT: @llvm.memcpy
----------------
Generally, I think we try to keep diag checking and codegen checking separate, but this is simple enough that I don't think it's a big deal.

Please use clang's `-verify` + `expected-warning` instead of `CHECK`s here.

Looks like, among others, test/CodeGen/const-init.c combines `-verify` with FileCheck'ing codegen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71082