[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:841
   operator.
-- ``clang_Cursor_getNumTemplateArguments``, ``clang_Cursor_getTemplateArgumentKind``, 
-  ``clang_Cursor_getTemplateArgumentType``, ``clang_Cursor_getTemplateArgumentValue`` and 
+- ``clang_Cursor_getNumTemplateArguments``, ``clang_Cursor_getTemplateArgumentKind``,
+  ``clang_Cursor_getTemplateArgumentType``, ``clang_Cursor_getTemplateArgumentValue`` and
----------------
unrelated changes here?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:541
+  let Content = [{
+A function declared with ``__attribute__((pure))`` attribute can not have
+infinite loops (i.e. they must either terminate or return to the caller,
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:544
+be it either via a normal return, or by throwing an exception and unwinding
+or exibiting UB), and can not cause any observable side-effects other than
+returning a value. They may read memory, but can not modify memory in a way
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:560
+
+A function declared with ``__attribute__((const))`` attribute can not have
+infinite loops (i.e. they must either terminate or return to the caller,
----------------
Similar changes to above.  This attribute is more of an 'assertion' by the developer that they promise they don't do these things (and we will UB otherwise), so I think they need to be written from that perspective.

I might suggest making an attempt to reword these docs.

ALSO, since these attributes can be spelled `clang::pure` and `clang::const` (IIRC?), I'd suggest we make the documents spelling-agnostic other than in direct code examples.


================
Comment at: clang/lib/Analysis/CFG.cpp:2725
       NoReturn = true;
-    if (FD->hasAttr<NoThrowAttr>())
+    if (FD->hasAttr<NoUnwindAttr>() || FD->hasAttr<NoThrowAttr>())
       AddEHEdge = false;
----------------
I find myself thinking we should probably have a function in FunctionDecl that tests for the various states of the function, rather than keep checking the attribute's presence.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958