I can see that this patch doesn't make much sense in isolation. This patch plus D66480 make an alternative to D66301. Hopefully the explanation in D66301 makes it clear that we're merely restoring previous behaviour.

It looks like if _LIBCPP_HAS_THREAD_API_PTHREAD is defined then with this change you'll be missing everything between lines 115 and 210.

In D66301#1636506, @zoecarver wrote:

Isn't that the intent? It looks like it's just conditionally adding an #include.

Hi @ruiu,
Can you comment on how this compares to clang-tidy? I had assumed that the readability-identifier-naming clang-tidy rule would largely do the trick.

@EricWF this change seems to have broken some a test somehow. I'm well out of my depth here so can you take a look?
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/25356/steps/test/logs/stdio

https://github.com/llvm/llvm-project/commit/d144572dac23c3af6a1ea5efbdffd8813138e216

LGTM

@hctim thanks for taking the time to point out the problems, sorry I missed them. I've looked again very carefully at all the buildbot output and came across another memory leak at ConstantsTest.cpp:482. Fingers crossed I've caught all the issues this time: rL355685

Hi @efriedma, sorry for the delayed response.
I've added the features you asked for to DataLayout.
It is intended that this patch preserves the existing behaviour if no function pointer alignment is specified.* Therefore I hope you'll agree that it isn't necessary for me to provide code to use the new feature on all platforms.
I did also look into using a target hook but doing so would require modifying a lot of functions to take a TargetTransformInfo argument. Given that we're in agreement that the DataLayout is an appropriate choice, it seemed like the best option.

In D57896#1402194, @lattner wrote:

Changed recommendation for acronyms from lower case to upper case, as suggested by several responses to the RFC.

I haven't been following the discussion closely - why is this the preferred direction? I don't think that things like "Basicblock *bb" or "MachineInstr *mi" will be confusing, and going towards a consistently leading lower case letter seems simple and preferable.

Changed recommendation for acronyms from lower case to upper case, as suggested by several responses to the RFC.

Update .clang-tidy files to use aNy_CasE until camelBackOrCase is available.
Add more guidance around acronyms.
Add more guidance around consistency with existing CamelCase variable names.
Change other code examples to camelBack.

In D54975#1391441, @mikhail.ramalho wrote:

@michaelplatings, thanks for the report.

That's weird, did you try to run the tests? You can do it with ninja check-clang-analyzer-z3

In D57896#1390517, @MyDeveloperDay wrote:

Should we come up with a new style? say UpperOrLowerCamelCase, I don't mind going and doing that in the readability-identifier-naming check, given that I just wrote up all the Options for that check https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html in D56563: [clang-tidy] add options documentation to readability-identifier-naming checker

In D57896#1389067, @lebedev.ri wrote:

1. Does clang-tidy warn on every single existing variable now?
2. It might be best to give this more visibility, by submitting a mail to llvm-dev, with a noticeable subject, like "RFC: changing variable naming rules in LLVM codebase"

In D57896#1389042, @lebedev.ri wrote:

Pretty sure this patch should have gone to llvm-commits, not cfe-commits.

Diff with -U99999

Diff with -U99999

Addressed @carwil's comments

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM