Just uploaded new diff as these files were reformatted as part of big python reformatting. To make sure whats in phabricator matches what lands.

In D150642#4344938, @nikic wrote:

LGTM. It may be helpful to include a pointer to alternative out-of-tree Python bindings in the patch description -- would https://github.com/numba/llvmlite be what people usually use?

In D144405#4184964, @erichkeane wrote:

So here's a potential idea for future development: It isn't uncommon/untypical for an attribute to want to return something other than '1', for 'version' (usually an integral value representing a date). The standard attributes all do this. It might be worth looking into some infrastructure to do that.

In D144402#4184749, @erichkeane wrote:

I want to wait for @aaron.ballman to answer, but i think this is generally OK. Note the motivation is the ability to check this from has_cpp_attribute/has_c_attribute/etc.

In D144405#4184726, @erichkeane wrote:

Looks like this doesn't compile pre-commit, though no idea if that is a patch-stack issue. Other than test, patch looks fine.

@erichkeane
Patch stack now updated to include a method extraction of hasSpelling, and releasenote added.

Updated diff with:

• Release note
• Cleaned up command line options in RUN commands
• Fixed type mimatch pointed out by @kimgr (out of band)

In D127201#3578077, @aaron.ballman wrote:

Thanks for adding some more test coverage! Would it be worth mentioning the miscompile fix in the release notes more explicitly?

One week gentle ping.
+ added reviewers from the https://reviews.llvm.org/D111400 patch that fixed the bug these tests covers.

Updated patch with full context.

Gentle 1-week ping

In D126578#3543994, @aaron.ballman wrote:

Thanks for looking to improve our test coverage here! However, precommit CI looks to be failing:

Added explicit triple to hopefully make it pass on win.
Made the new files clang-format clean.

Diff updated with release note + updated text in documentation as per jyknight's suggestion.

In D126324#3536785, @aaron.ballman wrote:

The changes so far look sensible, but I think we should add some more tests for a few situations. 1) Using a const weak symbol as a valid initializer should be diagnosed (with a warning? with an error?) so users are alerted to the behavioral quirks. 2) Using a const weak symbol in a constant expression context should probably be an error, right? e.g.,

Diff updated to be git-clang-format clean and to (hopefully) accommodate for differences in test output on w64.

This looks sensible to me. But I'm not an expert in the area (and still not 100% clear on the purpose of the ignorelist) so can't really say if the added comments captures the intent accurately.

There are other pragmas which include colon. How do they fare?

I think this looks good. I can't vouch for the cmake changes.

In D101139#2743346, @aabbaabb wrote:

Renamed lib to libexec and add custom rules to copy files to build output.

In D84090#2162389, @MyDeveloperDay wrote:

2. then I can do git clang-format, this will fix up any files in the diff that need formatting (you'll need to git add them again if they have)

Renamed option to "BitFieldColonSpacing" and made it an enum of: "Both", "None," "Before", "After"

In D84090#2160411, @curdeius wrote:

The changes look good to me in general. I share your doubt though about whether a bool flag is sufficient here. We've seen in the past a few times that at some time a false/true flag is not enough. I'd rather go for a Before/After/Both/None flag (or similar, naming probably should be coherent with other flags). But I'm not really aware of the projects/coding styles that use bit fields. Maybe a small research on this would be good to confirm or infirm a necessity of higher complexity.

• Regenerated rst
• Fixed clang format errors
• Added release note
• Added bool parsing test

In D68569#2019895, @MyDeveloperDay wrote:

Can we abandon this review now we have the style=file:<filename>?

In D68568#1735741, @MyDeveloperDay wrote:

Do you need help landing this?

The "Language" option can not distinguish between C and C++.

This depends on https://reviews.llvm.org/D68568

I might revisit this later. But carrying this patch locally for now.

Why are you interested in expending this list?

But running code that has been compiled with -mno-red-zone on a platform that guarantees that redzones are not stamped on should be fine as far as I can understand?