aaron.ballman added a comment.
It looks like you ran clang-format over the entire file in some instances,
causing a lot of unrelated changes to creep in. Can you back those changes out
and only run clang-format over just your patch instead
(http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting)?
================
Comment at: include/clang/Basic/Attr.td:436
@@ +435,3 @@
+ CXX11<"clang", "xray_never_instrument">];
+ let Subjects = SubjectList<[CXXMethod, ObjCMethod, Function], WarnDiag,
+ "ExpectedFunctionOrMethod">;
----------------
You can drop the WarnDiag and string literal; the defaults already do the right
thing.
================
Comment at: include/clang/Basic/AttrDocs.td:2455
@@ +2454,3 @@
+ let Content = [{
+``__attribute__((xray_always_instrument))`` or
``[[clang:xray_always_instrument]]`` is used to mark member functions (in C++),
methods (in Objective C), and free functions (in C, C++, and Objective C) to be
instrumented with XRay. This will cause the function to always have space at
the beginning and exit points to allow for runtime patching.
+
----------------
Let's make sure that this patch and D19908 do not conflict, because they seem
to be implementing competing semantics.
================
Comment at: lib/CodeGen/CodeGenFunction.cpp:381
@@ -380,3 +380,3 @@
- for (SmallVectorImpl<std::pair<llvm::Instruction *, llvm::Value *>
>::iterator
+ for (SmallVectorImpl<std::pair<llvm::Instruction *, llvm::Value *>>::iterator
I = DeferredReplacements.begin(),
----------------
Can't you rangify this loop?
================
Comment at: lib/CodeGen/CodeGenFunction.cpp:402
@@ +401,3 @@
+/// instrumented with XRay nop sleds.
+bool CodeGenFunction::ShouldXRayInstrumentFunction() {
+ return CGM.getCodeGenOpts().XRayInstrumentFunctions;
----------------
Any particular reason this isn't a `const` function?
================
Comment at: lib/CodeGen/CodeGenFunction.cpp:691
@@ +690,3 @@
+ if (D->hasAttr<XRayInstrumentAttr>()) {
+ switch (D->getAttr<XRayInstrumentAttr>()->getSpellingListIndex()) {
+ case 0:
----------------
This is not the right way to do this. ;-)
You should check the *semantic* spelling instead. e.g.,
```
if (const auto *A = D->getAttr<XRayInstrumentAttr>()) {
switch (A->getSemanticSpelling()) {
case XRayInstrumentAttr::GNU_blahblah:
case XRayInstrumentAttr::CXX11_clang_blahblah:
}
}
```
Alternatively (and I think this may be cleaner), you could add an additional
member in Attr.td that exposes this as two Boolean getters `bool
alwaysInstrument() const` and `bool neverInstrument() const` that do the
semantic spelling switch.
http://reviews.llvm.org/D20352
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
