[PATCH] D155081: Specify the developer policy around links to external resources

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

Looks like a great improvement to the policy, thank you all for sorting through 
this!


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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I would also love to see this conceptually, but think it will be pretty 
polarizing in the community.  It is worth another RFC thread before investing 
much time in it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155509: Revert "Remove rdar links; NFC"

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

Thank you, this should have been reverted immediately when concerns were raised.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155509

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Chris Lattner via Phabricator via cfe-commits]

lattner requested changes to this revision.
lattner added a comment.
This revision now requires changes to proceed.

Thank you so much for raising this issue Aaron.

Documenting the policy in the developer policy is totally the right thing to do 
given this cross-cuts individual subprojects, but I don't think we have 
consensus on what the right approach is.  This is a pretty important issue to a 
number of LLVM contributors, and I think we should have a first-principles 
discussion about it on the forum before instituting a policy.
https://discourse.llvm.org/t/code-review-reminder-about-links-in-code-commit-messages/71847/43?u=clattner


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150997: [llvm] Split out DenseMapInfo specialization

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

Awesome, this makes a lot of sense and is a great pattern to make these more 
modular.


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

https://reviews.llvm.org/D150997

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Got it this time, sorry for the confusion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I'm pretty sure I'm on top of commit access requests now, plz let me know if I 
missed you or something!  Could be spam filter or who knows what


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139705

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137960: [Lexer] Speedup LexTokenInternal

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

Nice!  I'd love someone else's eyes on this, but that's a great speedup!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137960

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131919: Move googletest to the third-party directory

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

I didn't review the patch in detail, but +1 this is a great step forward to 
reorganize the repo!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131919

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I haven't reviewed this in detail, but oh my I love LOVE LOVE this!  
`llvm::scc_traversal(x)` is so nice, great work!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126068: [llvm][clang][bolt][NFC] Use llvm::less_first() when applicable

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

This patch LGTM other than the apparent build breakage


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126068

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I'm not a competent reviewer for the patch, but +1 on aligning with GCC 
behavior here.  I don't recall the motivation for the original patch (apologies 
again for the terrible commit message).  This approach makes a lot of sense to 
me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125557: [APInt] Remove all uses of zextOrSelf, sextOrSelf and truncOrSelf

[Chris Lattner via Phabricator via cfe-commits]

lattner added inline comments.



Comment at: llvm/lib/Analysis/ConstantFolding.cpp:2884
 if (IntrinsicID == Intrinsic::smul_fix_sat) {
-  APInt Max = APInt::getSignedMaxValue(Width).sextOrSelf(ExtendedWidth);
-  APInt Min = APInt::getSignedMinValue(Width).sextOrSelf(ExtendedWidth);
+  APInt Max = APInt::getSignedMaxValue(Width).sext(ExtendedWidth);
+  APInt Min = APInt::getSignedMinValue(Width).sext(ExtendedWidth);

foad wrote:
> lattner wrote:
> > I think this can be a zext given the top bit will be zero
> Sure the first one could be zext, but the second one can't be, so it feels 
> conceptually simpler (to me) to keep them both as sext.
likewise, I'm fine with this either way.  zext is slightly more "Strength 
reduced" than sext, but it doesn't matter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125557

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125557: [APInt] Remove all uses of zextOrSelf, sextOrSelf and truncOrSelf

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

nice cleanup!




Comment at: llvm/lib/Analysis/ConstantFolding.cpp:2884
 if (IntrinsicID == Intrinsic::smul_fix_sat) {
-  APInt Max = APInt::getSignedMaxValue(Width).sextOrSelf(ExtendedWidth);
-  APInt Min = APInt::getSignedMinValue(Width).sextOrSelf(ExtendedWidth);
+  APInt Max = APInt::getSignedMaxValue(Width).sext(ExtendedWidth);
+  APInt Min = APInt::getSignedMinValue(Width).sext(ExtendedWidth);

I think this can be a zext given the top bit will be zero



Comment at: llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerCombiner.cpp:138
 
-  const APInt ConstValue = Const->Value.sextOrSelf(Ty.getSizeInBits());
+  const APInt ConstValue = Const->Value.sext(Ty.getSizeInBits());
   // The following code is ported from AArch64ISelLowering.

plz drop the extraneous 'const' while here.



Comment at: llvm/lib/Target/Hexagon/HexagonConstPropagation.cpp:1220
   if (Cmp & Comparison::U) {
-const APInt Zx1 = A1.zextOrSelf(MaxW);
-const APInt Zx2 = A2.zextOrSelf(MaxW);
+const APInt Zx1 = A1.zext(MaxW);
+const APInt Zx2 = A2.zext(MaxW);

Here too :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125557

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Chris Lattner via Phabricator via cfe-commits]

lattner edited reviewers, added: rsmith; removed: lattner.
lattner added a comment.

I'm not a competent reviewer for this, Richard can you recommend someone?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125402

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123957: Update the developer policy to mention release notes

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

Nice, LGTM!


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

https://reviews.llvm.org/D123957

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123957: Update the developer policy to mention release notes

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Also, when this lands, we should post on the forum about it.  Every change to 
the developer policy warrants broader visibility than just a phab discussion 
IMO.


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

https://reviews.llvm.org/D123957

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123957: Update the developer policy to mention release notes

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

This is awesome, I agree completely we should curate release notes better.  
That said, I think this should make it more clear that there is a "difference 
in kind" between user-facing tools like clang/lldb etc and other libraries in 
LLVM.  We don't want release note burden (or noise) for every little thing 
going into the optimizer or codegen.  Do you think it would make sense to point 
out that this is about user-facing tools?




Comment at: llvm/docs/DeveloperPolicy.rst:195
+* Adding, removing, or regrouping a diagnostic.
+* Fixing a bug (please link to the issue fixed in the bug database).
+* Adding or removing an optimization.

aaron.ballman wrote:
> nikic wrote:
> > I disagree with this point. Bug fixes should not make it into the release 
> > notes as a matter of course -- there may be specific high-impact bug fixes 
> > that may be worth mentioning, but bug fixes should not be included in the 
> > release notes as a matter of policy.
> > 
> > I agree that release notes for Clang/LLVM are currently insufficient, but 
> > we should also be careful not to over-compensate in the other direction, 
> > but including too much irrelevant noise.
> > I disagree with this point. Bug fixes should not make it into the release 
> > notes as a matter of course -- there may be specific high-impact bug fixes 
> > that may be worth mentioning, but bug fixes should not be included in the 
> > release notes as a matter of policy.
> 
> I disagree (quite strongly) with this and would point out that users have 
> explicitly requested this information be included: 
> https://discourse.llvm.org/t/rfc-update-developer-policy-on-release-notes/61856/3
> 
> > I agree that release notes for Clang/LLVM are currently insufficient, but 
> > we should also be careful not to over-compensate in the other direction, 
> > but including too much irrelevant noise.
> 
> Bug fixes are generally far from irrelevant, but I agree that reviewer and 
> author discretion is advised (as with any release note). For example, if you 
> introduce a bug in version N and fix the bug in version N, there's no need 
> for a release note in that situation because there's no user-facing change as 
> far as users care about. But I don't want to try to spell that out in precise 
> detail because there will always be edge cases.
I agree with both of you - listing every bug report would be too noisy and 
pretty irrelevant for most users, but there are high impact ones that are 
important and valuable to list.  How about wording this bullet something like:

* Fixing high impact bugs, e.g. ones that get discussed on hackernews or 
comparable forums (please link to the issue fixed in the bug database).



Comment at: llvm/docs/DeveloperPolicy.rst:196
+* Fixing a bug (please link to the issue fixed in the bug database).
+* Adding or removing an optimization.
+* Modifying a C stable API.

This is also too broad IMO, we don't want every new instcombine listed.  I'd 
recommend making this more user-centric, e.g. "Adding or removing optimizations 
that have widespread impact or enables new programming paradigms"


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

https://reviews.llvm.org/D123957

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123064: [Clang][C++23] P2071 Named universal character escapes

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Yeah, we should discuss this, thanks for raising this Aaron.  I'm not sure 
exactly what is being pulled in: @cor3ntin can you please email a summary of 
the situation to bo...@llvm.org and we'll discuss it and run it by Heather as 
needed?  Thanks

-Chris


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120713: [clangd] Make dexp command line options sticky

[Chris Lattner via Phabricator via cfe-commits]

lattner resigned from this revision.
lattner added a comment.
Herald added a project: All.

I'm not a competent reviewer for clangd


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120713

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Thank you so much Carlos, I hope they can come to see the value of having first 
class support for this in LLVM/Clang.  I really appreciate you pushing for this,

-Chris


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

https://reviews.llvm.org/D112730

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Hi all, I'm commenting on this based on my personal opinion, I don't speak for 
the LLVM board or anyone else.  I am also not a lawyer :)

This isn’t a clear cut case (as is typical!).  LLVM's approach on patents 
protection revolves primarily around the terms in the Apache 2 license, which 
is based on the owner of patents contributing code.  Beyond that, we 
can’t/don't proactively do all the research of potential patent infringement of 
contributed code.

That said, we have historically NOT taken code that is known to infringe on 
high risk patents.  The one that comes to mind is the (now expired) patents on 
Steensgaard’s unification-based pointer analysis work.  We rejected taking 
related work until those patents expire.

In this case, AUTOSAR/Parasoft looks like a little company, their spec is 
clearly saying they have IP rights associated with it, and the risks seem very 
high to me.  I think we should ask for a legal statement signed by a director 
of the company (not an informal email) saying we can use this in LLVM.  The 
risk is just otherwise too high for the vast community of people who use LLVM.

-Chris


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

https://reviews.llvm.org/D112730

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113641: [llvm] Add a SFINAE template parameter to DenseMapInfo

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Oh, are you concerned about staging this in?  If you want to stage it (add the 
includes now, remove them later or something), that is totally fine with me.  
Maybe I don't understand the impact correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113641

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113641: [llvm] Add a SFINAE template parameter to DenseMapInfo

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I think a few void's probably won't hurt anyone?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113641

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113641: [llvm] Add a SFINAE template parameter to DenseMapInfo

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
Herald added a subscriber: sdasgup3.

Nice, I'm very excited about this - this will allow a lot of cleanups.  Thank 
you for doing this!




Comment at: llvm/include/llvm/ADT/Hashing.h:59
 namespace llvm {
-template  struct DenseMapInfo;
 

Is there a way to keep the forward declarations references here instead of 
#include?  DenseMapInfo.h pulls in a ton of stuff including  and 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113641

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113080: [Support] Improve Caching conformance with Support library behavior

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

nice, thank you for improving this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113080

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111049: [Support] Change fatal_error_handler_t to take a const char* instead of std::string

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

This is really great, thank you for doing this.  I'd love to see  
reduced out of headers!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111049

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110808: [APInt] Stop using soft-deprecated constructors and methods in clang. NFC.

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

Thank you Jay!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110808

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Chris Lattner via Phabricator via cfe-commits]

lattner resigned from this revision.
lattner edited reviewers, added: rsmith; removed: lattner.
lattner added a comment.

I'm not sure who the best person is to review this, but it isn't me anymore 
sadly.  Richard, can you recommend someone?




Comment at: clang/lib/Basic/Sarif.cpp:1
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/LangOptions.h"

THis nees the standard header boilerplate per the coding standards doc



Comment at: clang/lib/Basic/Sarif.cpp:27
+
+StringRef getFileName(const FileEntry ) {
+  StringRef Filename = FE.tryGetRealPathName();

vaibhav.y wrote:
> A lot of these are copied from [[ 
> https://github.com/llvm/llvm-project/blob/181d18ef53db1e5810bf6b905fbafc91da9b5baa/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp#L64
>  | SarifDiagnostics.cpp ]]
Please use static methods instead of anonymous namespaces, per the coding 
standards doc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

> What is a "keep constructor"?

Good question, I'm not sure.  I think I meant to say "key constructors".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I'll take care of the DAG.getAllOnesConstant change, but i'd appreciate it if 
you could look at the NOT cases.  Running tests on the DAG.getAllOnesConstant 
patch now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Thank you for the detailed review Craig!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

[Chris Lattner via Phabricator via cfe-commits]

lattner added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3243
"Don't know how to expand this subtraction!");
-Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1),
-   DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
-   VT));
+Tmp1 = DAG.getNode(
+ISD::XOR, dl, VT, Node->getOperand(1),

craig.topper wrote:
> This could use DAG.getNOT if you're willing to make that change.
I'd prefer to keep this one separate, it isn't related to APInt.



Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:12185
 else
-  OtherOp = DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
-VT);
+  OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
 return true;

craig.topper wrote:
> I think we have DAG.getAllOnesConstant if you want to use it
Will do this in a followup



Comment at: llvm/lib/Target/Lanai/LanaiISelLowering.cpp:1403
 else
-  OtherOp =
-  DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl, VT);
+  OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
 return true;

craig.topper wrote:
> I think we have DAG.getAllOnesConstant if you want to use it
Likewise



Comment at: llvm/unittests/ADT/APIntTest.cpp:29
 TEST(APIntTest, ShiftLeftByZero) {
-  APInt One = APInt::getNullValue(65) + 1;
+  APInt One = APInt::getZero(65) + 1;
   APInt Shl = One.shl(0);

craig.topper wrote:
> That's a strange way to write APInt(64, 1) unless there was some specific bug.
I agree, assume that this was testing some former bug.  Unit tests are designed 
to be weird ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

[Chris Lattner via Phabricator via cfe-commits]

lattner marked an inline comment as done.
lattner added inline comments.



Comment at: llvm/include/llvm/ADT/APInt.h:384
   /// value for the APInt's bit width.
   bool isMaxValue() const { return isAllOnesValue(); }
 

craig.topper wrote:
> isAllOnes()?
Yep, good catch, fixed thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

This patch has a lot of noise, the important part is APInt.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107528: [llvm][clang][NFC] updates inline licence info

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

Thank you for catching this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107528

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

> @lattner, thanks for the help. In this case, the real question is whether 
> there's any use case for !srcloc that involves writing it out into a bitcode 
> or IR file and then having a separate instance of clang load it back in again.



> I think that no such case can possibly give a useful mapping back to the 
> original source, because IR doesn't serialise the SourceManager that knows 
> how to turn source locations back into a useful error message.

Right, the only use case is with the entire compiler integrated together, for 
exactly the reason you point out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105491

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

  > cat f.c
  
  void x() {
asm("bogus");
  }
  > clang f.c
  f.c:3:7: error: invalid instruction mnemonic 'bogus'
asm("bogus");
^
  :1:2: note: instantiated into assembly here
  bogus
  ^
  1 error generated.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105491

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

The usecase for this is to get good diagnostics out of the integrated 
assembler.  Try doing something like: `asm("bogus");`,  GCC would produce an 
error pointing to the generated .s file.  Clang should point to the C file 
(including macros expanded etc).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105491

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

[Chris Lattner via Phabricator via cfe-commits]

lattner added inline comments.



Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600
+  if (RHS.data() == getEmptyKey().data())
+return LHS.data() == getEmptyKey().data();
+  if (RHS.data() == getTombstoneKey().data())
+return LHS.data() == getTombstoneKey().data();
+  return LHS == RHS;
+}

nikic wrote:
> lattner wrote:
> > craig.topper wrote:
> > > lattner wrote:
> > > > I'm pretty sure this method can just be "return LHS == RHS;"  The 
> > > > tombstone/empty comparisons should work without special cases.
> > > Doesn't operator== on ArrayRef compare the elements of the arrays? So 
> > > wouldn't that dereference the invalid pointers used by tombstone/empty?
> > Yes, implemented in terms of std::equal.  However, both of these cases have 
> > zero elements, so the "pointer" is never dereferenced.
> As the length is zero, wouldn't the empty key, the tombstone key and an empty 
> ArrayRef all be considered equal, as the data pointer is never compared?
Yes, you're right, this won't work.  Good catch, thanks :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

[Chris Lattner via Phabricator via cfe-commits]

lattner added inline comments.



Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600
+  if (RHS.data() == getEmptyKey().data())
+return LHS.data() == getEmptyKey().data();
+  if (RHS.data() == getTombstoneKey().data())
+return LHS.data() == getTombstoneKey().data();
+  return LHS == RHS;
+}

craig.topper wrote:
> lattner wrote:
> > I'm pretty sure this method can just be "return LHS == RHS;"  The 
> > tombstone/empty comparisons should work without special cases.
> Doesn't operator== on ArrayRef compare the elements of the arrays? So 
> wouldn't that dereference the invalid pointers used by tombstone/empty?
Yes, implemented in terms of std::equal.  However, both of these cases have 
zero elements, so the "pointer" is never dereferenced.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

> This required adding quite a few additional includes, as many files were 
> relying on various things pulled in by ArrayRef.h.

This is a _good_ thing btw!

Thank you for driving this!




Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600
+  if (RHS.data() == getEmptyKey().data())
+return LHS.data() == getEmptyKey().data();
+  if (RHS.data() == getTombstoneKey().data())
+return LHS.data() == getTombstoneKey().data();
+  return LHS == RHS;
+}

I'm pretty sure this method can just be "return LHS == RHS;"  The 
tombstone/empty comparisons should work without special cases.



Comment at: llvm/lib/CodeGen/MBFIWrapper.cpp:14
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/CodeGen/MBFIWrapper.h"

This clang format warning is right, plz move under the other #include



Comment at: llvm/lib/Support/SmallPtrSet.cpp:15
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/Support/MathExtras.h"

These are also right, plz fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103491

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97320: Use a fast path when initializing LineOffsetMapping

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Nice, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97320

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92269: [TableGen] Eliminate the 'code' type

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

Woot!  Go Paul!


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

https://reviews.llvm.org/D92269

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92269: [TableGen] Eliminate the 'code' type

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Oh ok, well I have no problem keeping it around in searchable tables, I misread 
this as making that more general.

jrtc27, we had a long conversation about this.  We'd like to eliminate the Code 
type as a way to simplify the internals of tblgen.  It is a distinction without 
a useful difference for many things, and this is one step towards making tblgen 
simpler and more consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92269

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92269: [TableGen] Eliminate the 'code' type

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Ah, I thought TypeOf_xxx was a transitionary thing that we'd drop over time (to 
reduce complexity).  If that's the case, I'd recommend not documenting it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92269

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89893: [Clang] [TableGen] Clean up !if(!eq(boolean, 1) and related booleans

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Oops, thanks for the head's up @dblaikie !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89893

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36836: [clang-tidy] Implement sonarsource-function-cognitive-complexity check

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Hi all,

The LLVM foundation board discussed this offline -- it looks like the 
contributor is a bit confused about the LLVM project policies towards 
copyright, license, patent stuff etc.  I really appreciate the dilligence here 
to capture the ideas that went into this code.

My understanding is that the code and documentation are not derived from anyone 
else's code or documentation -- they are fresh implementations based on a paper 
written in english prose.  If that is the case, please remove the LICENSE.TXT 
file -- it isn't necessary and it is confusing for people.I would also 
recommend replacing all the references to sonar source with a functional 
description of what this patch is doing - we aren't talking about adding a 
feature because of sonar source, my understanding is that this is adding a 
cyclomatic complexity checker.  If and when the feature evolves over time, it 
would be misleading for it to be called sonar source.

Please note that I didn't do a full code review here, others should do that.

-Chris


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36836

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Chris Lattner via Phabricator via cfe-commits]

lattner resigned from this revision.
lattner added a comment.

Please don't consider me a blocker on this patch, thank you for pushing on it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Chris Lattner via Phabricator via cfe-commits]

lattner requested changes to this revision.
lattner added a comment.
This revision now requires changes to proceed.

This looks like a great direction, but please make sure to minimize public 
implementation details.  We don't want the vast majority of instcombine to be 
visible outside of its library (it is hairy enough as it is :-)




Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:29
 #include "llvm/Support/DataTypes.h"
+#include "llvm/Support/KnownBits.h"
 #include 

Can this be forward declared instead of #include'd?



Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:30
+#include "llvm/Transforms/Utils/Local.h"
+#include 
+

Please minimize #includes in general, thanks :)



Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:46
+/// combine them.
+class LLVM_LIBRARY_VISIBILITY InstCombiner {
+public:

I would really rather not make this be a public class - this is a very thick 
interface.  Can this be cut down to something much smaller than the 
implementation details of InstCombine?

If you're curious for a pattern that could be followed, the MLIR AsmParser is a 
reasonable example.  The parser is spread across a bunch of classes in the lib/ 
directory:
https://github.com/llvm/llvm-project/blob/master/mlir/lib/Parser/Parser.cpp

But then there is a much smaller public API exposed through a header:
https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/IR/OpImplementation.h#L229




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82259: Deprecate error prone temporary directory APIs

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Hi Dave,

This is doing two things - changing the behavior of clang (where it stores its 
module cache) and deprecating an API.  I'd recommend we start by focusing on 
whether the first part is the right thing to do, and just remove the API if 
this is the only client of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82259



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78870: [SourceMgr] Tidy up the SourceMgr header file to include less stuff.

[Chris Lattner via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG919dcc7f6858: [SourceMgr] Tidy up the SourceMgr header file 
to include less stuff. (authored by lattner).

Changed prior to commit:
  https://reviews.llvm.org/D78870?vs=260140=260141#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78870

Files:
  clang/include/clang/Basic/FileManager.h
  llvm/include/llvm/Support/SourceMgr.h
  llvm/lib/Support/SourceMgr.cpp

Index: llvm/lib/Support/SourceMgr.cpp
===
--- llvm/lib/Support/SourceMgr.cpp
+++ llvm/lib/Support/SourceMgr.cpp
@@ -69,16 +69,13 @@
 }
 
 template 
-static std::vector (
-PointerUnion *, std::vector *,
- std::vector *, std::vector *> ,
-MemoryBuffer *Buffer) {
-  if (!OffsetCache.isNull())
-return *OffsetCache.get *>();
+static std::vector (void *,
+  MemoryBuffer *Buffer) {
+  if (OffsetCache)
+return *static_cast *>(OffsetCache);
 
   // Lazily fill in the offset cache.
   auto *Offsets = new std::vector();
-  OffsetCache = Offsets;
   size_t Sz = Buffer->getBufferSize();
   assert(Sz <= std::numeric_limits::max());
   StringRef S = Buffer->getBuffer();
@@ -87,6 +84,7 @@
   Offsets->push_back(static_cast(N));
   }
 
+  OffsetCache = Offsets;
   return *Offsets;
 }
 
@@ -164,15 +162,16 @@
 }
 
 SourceMgr::SrcBuffer::~SrcBuffer() {
-  if (!OffsetCache.isNull()) {
-if (OffsetCache.is *>())
-  delete OffsetCache.get *>();
-else if (OffsetCache.is *>())
-  delete OffsetCache.get *>();
-else if (OffsetCache.is *>())
-  delete OffsetCache.get *>();
+  if (OffsetCache) {
+size_t Sz = Buffer->getBufferSize();
+if (Sz <= std::numeric_limits::max())
+  delete static_cast *>(OffsetCache);
+else if (Sz <= std::numeric_limits::max())
+  delete static_cast *>(OffsetCache);
+else if (Sz <= std::numeric_limits::max())
+  delete static_cast *>(OffsetCache);
 else
-  delete OffsetCache.get *>();
+  delete static_cast *>(OffsetCache);
 OffsetCache = nullptr;
   }
 }
@@ -329,6 +328,15 @@
 }
 
 //===--===//
+// SMFixIt Implementation
+//===--===//
+
+SMFixIt::SMFixIt(SMRange R, const Twine )
+: Range(R), Text(Replacement.str()) {
+  assert(R.isValid());
+}
+
+//===--===//
 // SMDiagnostic Implementation
 //===--===//
 
Index: llvm/include/llvm/Support/SourceMgr.h
===
--- llvm/include/llvm/Support/SourceMgr.h
+++ llvm/include/llvm/Support/SourceMgr.h
@@ -15,19 +15,9 @@
 #ifndef LLVM_SUPPORT_SOURCEMGR_H
 #define LLVM_SUPPORT_SOURCEMGR_H
 
-#include "llvm/ADT/ArrayRef.h"
-#include "llvm/ADT/None.h"
-#include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Twine.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SMLoc.h"
-#include 
-#include 
-#include 
-#include 
-#include 
 #include 
 
 namespace llvm {
@@ -57,21 +47,17 @@
 /// The memory buffer for the file.
 std::unique_ptr Buffer;
 
-/// Helper type for OffsetCache below: since we're storing many offsets
-/// into relatively small files (often smaller than 2^8 or 2^16 bytes),
-/// we select the offset vector element type dynamically based on the
-/// size of Buffer.
-using VariableSizeOffsets =
-PointerUnion *, std::vector *,
- std::vector *, std::vector *>;
-
 /// Vector of offsets into Buffer at which there are line-endings
 /// (lazily populated). Once populated, the '\n' that marks the end of
 /// line number N from [1..] is at Buffer[OffsetCache[N-1]]. Since
 /// these offsets are in sorted (ascending) order, they can be
 /// binary-searched for the first one after any given offset (eg. an
 /// offset corresponding to a particular SMLoc).
-mutable VariableSizeOffsets OffsetCache;
+///
+/// Since we're storing offsets into relatively small files (often smaller
+/// than 2^8 or 2^16 bytes), we select the offset vector element type
+/// dynamically based on the size of Buffer.
+mutable void *OffsetCache = nullptr;
 
 /// Look up a given \p Ptr in in the buffer, determining which line it came
 /// from.
@@ -196,14 +182,14 @@
   /// \param ShowColors Display colored messages if output is a terminal and
   /// the default error handler is used.
   void PrintMessage(raw_ostream , SMLoc Loc, DiagKind Kind, const Twine ,
-   

[PATCH] D78870: [SourceMgr] Tidy up the SourceMgr header file to include less stuff.

[Chris Lattner via Phabricator via cfe-commits]

lattner updated this revision to Diff 260140.
lattner added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Clang depended on the transitive include, update it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78870

Files:
  clang/include/clang/Basic/FileManager.h
  llvm/include/llvm/Support/SourceMgr.h
  llvm/lib/Support/SourceMgr.cpp

Index: llvm/lib/Support/SourceMgr.cpp
===
--- llvm/lib/Support/SourceMgr.cpp
+++ llvm/lib/Support/SourceMgr.cpp
@@ -69,16 +69,13 @@
 }
 
 template 
-static std::vector (
-PointerUnion *, std::vector *,
- std::vector *, std::vector *> ,
-MemoryBuffer *Buffer) {
-  if (!OffsetCache.isNull())
-return *OffsetCache.get *>();
+static std::vector (void *,
+  MemoryBuffer *Buffer) {
+  if (OffsetCache)
+return *static_cast *>(OffsetCache);
 
   // Lazily fill in the offset cache.
   auto *Offsets = new std::vector();
-  OffsetCache = Offsets;
   size_t Sz = Buffer->getBufferSize();
   assert(Sz <= std::numeric_limits::max());
   StringRef S = Buffer->getBuffer();
@@ -87,6 +84,7 @@
   Offsets->push_back(static_cast(N));
   }
 
+  OffsetCache = Offsets;
   return *Offsets;
 }
 
@@ -164,15 +162,16 @@
 }
 
 SourceMgr::SrcBuffer::~SrcBuffer() {
-  if (!OffsetCache.isNull()) {
-if (OffsetCache.is *>())
-  delete OffsetCache.get *>();
-else if (OffsetCache.is *>())
-  delete OffsetCache.get *>();
-else if (OffsetCache.is *>())
-  delete OffsetCache.get *>();
+  if (OffsetCache) {
+size_t Sz = Buffer->getBufferSize();
+if (Sz <= std::numeric_limits::max())
+  delete static_cast *>(OffsetCache);
+else if (Sz <= std::numeric_limits::max())
+  delete static_cast *>(OffsetCache);
+else if (Sz <= std::numeric_limits::max())
+  delete static_cast *>(OffsetCache);
 else
-  delete OffsetCache.get *>();
+  delete static_cast *>(OffsetCache);
 OffsetCache = nullptr;
   }
 }
@@ -328,6 +327,15 @@
   PrintMessage(errs(), Loc, Kind, Msg, Ranges, FixIts, ShowColors);
 }
 
+//===--===//
+// SMFixIt Implementation
+//===--===//
+
+SMFixIt::SMFixIt(SMRange R, const Twine )
+: Range(R), Text(Replacement.str()) {
+  assert(R.isValid());
+}
+
 //===--===//
 // SMDiagnostic Implementation
 //===--===//
Index: llvm/include/llvm/Support/SourceMgr.h
===
--- llvm/include/llvm/Support/SourceMgr.h
+++ llvm/include/llvm/Support/SourceMgr.h
@@ -15,19 +15,9 @@
 #ifndef LLVM_SUPPORT_SOURCEMGR_H
 #define LLVM_SUPPORT_SOURCEMGR_H
 
-#include "llvm/ADT/ArrayRef.h"
-#include "llvm/ADT/None.h"
-#include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Twine.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SMLoc.h"
-#include 
-#include 
-#include 
-#include 
-#include 
 #include 
 
 namespace llvm {
@@ -57,21 +47,17 @@
 /// The memory buffer for the file.
 std::unique_ptr Buffer;
 
-/// Helper type for OffsetCache below: since we're storing many offsets
-/// into relatively small files (often smaller than 2^8 or 2^16 bytes),
-/// we select the offset vector element type dynamically based on the
-/// size of Buffer.
-using VariableSizeOffsets =
-PointerUnion *, std::vector *,
- std::vector *, std::vector *>;
-
 /// Vector of offsets into Buffer at which there are line-endings
 /// (lazily populated). Once populated, the '\n' that marks the end of
 /// line number N from [1..] is at Buffer[OffsetCache[N-1]]. Since
 /// these offsets are in sorted (ascending) order, they can be
 /// binary-searched for the first one after any given offset (eg. an
 /// offset corresponding to a particular SMLoc).
-mutable VariableSizeOffsets OffsetCache;
+///
+/// Since we're storing offsets into relatively small files (often smaller
+/// than 2^8 or 2^16 bytes), we select the offset vector element type
+/// dynamically based on the size of Buffer.
+mutable void *OffsetCache = nullptr;
 
 /// Look up a given \p Ptr in in the buffer, determining which line it came
 /// from.
@@ -196,14 +182,14 @@
   /// \param ShowColors Display colored messages if output is a terminal and
   /// the default error handler is used.
   void PrintMessage(raw_ostream , SMLoc Loc, DiagKind Kind, const Twine ,
-ArrayRef Ranges = None,
-ArrayRef FixIts = None,
+

[PATCH] D78273: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet.

[Chris Lattner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG39c9c12b76da: [clang-tools-extra] reimplement 
PreprocessorTracker in terms of StringSet. (authored by lattner).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78273

Files:
  clang-tools-extra/modularize/PreprocessorTracker.cpp

Index: clang-tools-extra/modularize/PreprocessorTracker.cpp
===
--- clang-tools-extra/modularize/PreprocessorTracker.cpp
+++ clang-tools-extra/modularize/PreprocessorTracker.cpp
@@ -243,19 +243,19 @@
 //
 //======//
 
-#include "clang/Lex/LexDiagnostic.h"
 #include "PreprocessorTracker.h"
+#include "ModularizeUtilities.h"
+#include "clang/Lex/LexDiagnostic.h"
 #include "clang/Lex/MacroArgs.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "llvm/ADT/SmallSet.h"
-#include "llvm/Support/StringPool.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/raw_ostream.h"
-#include "ModularizeUtilities.h"
 
 namespace Modularize {
 
 // Some handle types
-typedef llvm::PooledStringPtr StringHandle;
+typedef llvm::StringRef StringHandle;
 
 typedef int HeaderHandle;
 const HeaderHandle HeaderHandleInvalid = -1;
@@ -463,19 +463,6 @@
   "(not evaluated)", "false", "true"
 };
 
-bool operator<(const StringHandle , const StringHandle ) {
-  const char *S1 = (H1 ? *H1 : "");
-  const char *S2 = (H2 ? *H2 : "");
-  int Diff = strcmp(S1, S2);
-  return Diff < 0;
-}
-bool operator>(const StringHandle , const StringHandle ) {
-  const char *S1 = (H1 ? *H1 : "");
-  const char *S2 = (H2 ? *H2 : "");
-  int Diff = strcmp(S1, S2);
-  return Diff > 0;
-}
-
 // Preprocessor item key.
 //
 // This class represents a location in a source file, for use
@@ -922,7 +909,9 @@
   }
 
   // Lookup/add string.
-  StringHandle addString(llvm::StringRef Str) { return Strings.intern(Str); }
+  StringHandle addString(llvm::StringRef Str) {
+return Strings.insert(Str).first->first();
+  }
 
   // Convert to a canonical path.
   std::string getCanonicalPath(llvm::StringRef path) const {
@@ -950,7 +939,7 @@
 HeaderHandle H = 0;
 for (auto I = HeaderPaths.begin(), E = HeaderPaths.end(); I != E;
  ++I, ++H) {
-  if (**I == CanonicalPath)
+  if (*I == CanonicalPath)
 return H;
 }
 return HeaderHandleInvalid;
@@ -1143,10 +1132,10 @@
   // Tell caller we found one or more errors.
   ReturnValue = true;
   // Start the error message.
-  OS << *MacroExpTracker.InstanceSourceLine;
+  OS << MacroExpTracker.InstanceSourceLine;
   if (ItemKey.Column > 0)
 OS << std::string(ItemKey.Column - 1, ' ') << "^\n";
-  OS << "error: Macro instance '" << *MacroExpTracker.MacroUnexpanded
+  OS << "error: Macro instance '" << MacroExpTracker.MacroUnexpanded
  << "' has different values in this header, depending on how it was "
 "included.\n";
   // Walk all the instances.
@@ -1154,8 +1143,8 @@
 EMT = MacroExpTracker.MacroExpansionInstances.end();
IMT != EMT; ++IMT) {
 MacroExpansionInstance  = *IMT;
-OS << "  '" << *MacroExpTracker.MacroUnexpanded << "' expanded to: '"
-   << *MacroInfo.MacroExpanded
+OS << "  '" << MacroExpTracker.MacroUnexpanded << "' expanded to: '"
+   << MacroInfo.MacroExpanded
<< "' with respect to these inclusion paths:\n";
 // Walk all the inclusion path hierarchies.
 for (auto IIP = MacroInfo.InclusionPathHandles.begin(),
@@ -1165,7 +1154,7 @@
   auto Count = (int)ip.size();
   for (int Index = 0; Index < Count; ++Index) {
 HeaderHandle H = ip[Index];
-OS << std::string((Index * 2) + 4, ' ') << *getHeaderFilePath(H)
+OS << std::string((Index * 2) + 4, ' ') << getHeaderFilePath(H)
<< "\n";
   }
 }
@@ -1173,7 +1162,7 @@
 // instance location.
 // If there is a definition...
 if (MacroInfo.DefinitionLocation.Line != ItemKey.Line) {
-  OS << *MacroInfo.DefinitionSourceLine;
+  OS << MacroInfo.DefinitionSourceLine;
   if (MacroInfo.DefinitionLocation.Column > 0)
 OS << std::string(MacroInfo.DefinitionLocation.Column - 1, ' ')
<< "^\n";
@@ -1201,13 +1190,13 @@
   // Tell caller we found one or more errors.
   ReturnValue = true;
   // Start the error message.
-  OS << *HeaderPaths[ItemKey.File] << ":" << ItemKey.Line << ":"
+  OS << HeaderPaths[ItemKey.File] << ":" << ItemKey.Line << ":"
  << ItemKey.Column << "\n";
   OS << "#" << getDirectiveSpelling(CondTracker.DirectiveKind) << " "
- << *CondTracker.ConditionUnexpanded << "\n";
+ << CondTracker.ConditionUnexpanded << "\n";
   OS << "^\n";
   OS << 

[PATCH] D78273: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet.

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Thank you for the review Reid!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78273



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78273: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet.

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Hey @maskray, yes StringSet/StringMap's entries are separately allocated and 
the iterators/pointers are stable.  The strings are held in each entry.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78273



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78273: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet.

[Chris Lattner via Phabricator via cfe-commits]

lattner created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
MaskRay added inline comments.



Comment at: clang-tools-extra/modularize/PreprocessorTracker.cpp:912
   // Lookup/add string.
-  StringHandle addString(llvm::StringRef Str) { return Strings.intern(Str); }
+  StringHandle addString(llvm::StringRef Str) {
+return Strings.insert(Str).first->first();

Is it well-known that a StringSet (= `StringMap`) returned 
StringRef is stable? Is that property something we can reliably depend on?

If not, StringSaver.h:UniqueStringSaver may be a better choice.


PreprocessorTracker is the last user of the old StringPool class, which
isn't super loved and isn't a great improvement over a plan StringSet.
Once this goes in we can remove StringPool entirely.

This is as discussed on cfe-dev.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78273

Files:
  clang-tools-extra/modularize/PreprocessorTracker.cpp

Index: clang-tools-extra/modularize/PreprocessorTracker.cpp
===
--- clang-tools-extra/modularize/PreprocessorTracker.cpp
+++ clang-tools-extra/modularize/PreprocessorTracker.cpp
@@ -243,19 +243,19 @@
 //
 //======//
 
-#include "clang/Lex/LexDiagnostic.h"
 #include "PreprocessorTracker.h"
+#include "ModularizeUtilities.h"
+#include "clang/Lex/LexDiagnostic.h"
 #include "clang/Lex/MacroArgs.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "llvm/ADT/SmallSet.h"
-#include "llvm/Support/StringPool.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/raw_ostream.h"
-#include "ModularizeUtilities.h"
 
 namespace Modularize {
 
 // Some handle types
-typedef llvm::PooledStringPtr StringHandle;
+typedef llvm::StringRef StringHandle;
 
 typedef int HeaderHandle;
 const HeaderHandle HeaderHandleInvalid = -1;
@@ -463,19 +463,6 @@
   "(not evaluated)", "false", "true"
 };
 
-bool operator<(const StringHandle , const StringHandle ) {
-  const char *S1 = (H1 ? *H1 : "");
-  const char *S2 = (H2 ? *H2 : "");
-  int Diff = strcmp(S1, S2);
-  return Diff < 0;
-}
-bool operator>(const StringHandle , const StringHandle ) {
-  const char *S1 = (H1 ? *H1 : "");
-  const char *S2 = (H2 ? *H2 : "");
-  int Diff = strcmp(S1, S2);
-  return Diff > 0;
-}
-
 // Preprocessor item key.
 //
 // This class represents a location in a source file, for use
@@ -922,7 +909,9 @@
   }
 
   // Lookup/add string.
-  StringHandle addString(llvm::StringRef Str) { return Strings.intern(Str); }
+  StringHandle addString(llvm::StringRef Str) {
+return Strings.insert(Str).first->first();
+  }
 
   // Convert to a canonical path.
   std::string getCanonicalPath(llvm::StringRef path) const {
@@ -950,7 +939,7 @@
 HeaderHandle H = 0;
 for (auto I = HeaderPaths.begin(), E = HeaderPaths.end(); I != E;
  ++I, ++H) {
-  if (**I == CanonicalPath)
+  if (*I == CanonicalPath)
 return H;
 }
 return HeaderHandleInvalid;
@@ -1143,10 +1132,10 @@
   // Tell caller we found one or more errors.
   ReturnValue = true;
   // Start the error message.
-  OS << *MacroExpTracker.InstanceSourceLine;
+  OS << MacroExpTracker.InstanceSourceLine;
   if (ItemKey.Column > 0)
 OS << std::string(ItemKey.Column - 1, ' ') << "^\n";
-  OS << "error: Macro instance '" << *MacroExpTracker.MacroUnexpanded
+  OS << "error: Macro instance '" << MacroExpTracker.MacroUnexpanded
  << "' has different values in this header, depending on how it was "
 "included.\n";
   // Walk all the instances.
@@ -1154,8 +1143,8 @@
 EMT = MacroExpTracker.MacroExpansionInstances.end();
IMT != EMT; ++IMT) {
 MacroExpansionInstance  = *IMT;
-OS << "  '" << *MacroExpTracker.MacroUnexpanded << "' expanded to: '"
-   << *MacroInfo.MacroExpanded
+OS << "  '" << MacroExpTracker.MacroUnexpanded << "' expanded to: '"
+   << MacroInfo.MacroExpanded
<< "' with respect to these inclusion paths:\n";
 // Walk all the inclusion path hierarchies.
 for (auto IIP = MacroInfo.InclusionPathHandles.begin(),
@@ -1165,7 +1154,7 @@
   auto Count = (int)ip.size();
   for (int Index = 0; Index < Count; ++Index) {
 HeaderHandle H = ip[Index];
-OS << std::string((Index * 2) + 4, ' ') << *getHeaderFilePath(H)
+OS << std::string((Index * 2) + 4, ' ') << getHeaderFilePath(H)
<< "\n";
   }
 }
@@ -1173,7 +1162,7 @@
 // instance location.
 // If there is a definition...
 if (MacroInfo.DefinitionLocation.Line != ItemKey.Line) {
-  OS << *MacroInfo.DefinitionSourceLine;
+  OS << MacroInfo.DefinitionSourceLine;
   if (MacroInfo.DefinitionLocation.Column > 0)
 OS << 

[PATCH] D77683: [Docs] Make code review policy clearer about requested pre-commit reviews

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I don't really understand what this is getting at, so I'd recommend rewording 
this a bit for clarity.  Would something along the lines of this capture the 
intended meaning?:

If there is likely to be uncertainty, you should default to getting a patch 
reviewed prior to commit, particularly if someone has asked for extra review of 
a specific area.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77683



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75169: [ARM] Enforcing calling convention for half-precision FP arguments and returns for big-endian AArch32

[Chris Lattner via Phabricator via cfe-commits]

lattner resigned from this revision.
lattner added a comment.

I'm not a qualified reviewer for this at this point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75169



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I'd be happy to help fix that problem.  Please take a look at the llvm 
developer policy. :-)


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

https://reviews.llvm.org/D74801



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

Seems fine to me.


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

https://reviews.llvm.org/D74801



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

I'm super excited to see this progress towards supporting 'asm goto' with 
results!  Great work!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

In D57896#1434877 , @Charusso wrote:

> static Optional
>  getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
>  //...
>
>   if (const auto *DRE = dyn_cast_or_null(CondVarExpr)) {
> if (const auto *VD = dyn_cast_or_null(DRE->getDecl())) {
>
> //...
>  }
>
>   would be:
>  
>
>
> static Optional 
>   |
>  getConcreteIntegerValue(const Expr *cond_var_expr, const ExplodedNode *node) 
> {  |
>  //...
>|
>
>   if (const auto *decl_ref_expr = 
> dyn_cast_or_null(cond_var_expr)) {
> if (const auto *var_decl = 
> dyn_cast_or_null(decl_ref_expr->getDecl())) {
>
> //... 
>   |
>  } whoops 
> column-81 ~^
>
>   Hungarian notation on members and globals are cool idea. However, the 
> notation is made without the `_` part, so I think `mMember` is better than 
> `m_member` as we used to 80-column standard and it is waste of space and 
> hurts your C-developer eyes. I would recommend `b` prefix to booleans as 
> Unreal Engine 4 styling is used to do that (`bIsCoolStyle`) and it is handy. 
> It is useful because booleans usually has multiple prefixes: `has, have, is` 
> and you would list all the booleans together in autocompletion. Yes, there is 
> a problem: if the notation is not capital like the pure Hungarian notation 
> then it is problematic to list and we are back to the `BIsCapitalLetter` and 
> `MMember` CamelCase-world where we started (except one project). I think 
> @lattner could say if it is useful as all the Apple projects based on those 
> notations and could be annoying.


FWIW, my suggestion is *not* to expand names like DRE to decl_ref_expr, I agree 
that doesn't add clarity to the code.  Two possibilities: "dre", or "decl" 
which is what I would write today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I can understand Zach's position here, but LLDB has historically never 
conformed to the general LLVM naming or other conventions due to its heritage.  
It should not be taken as precedent that the rest of the project should follow.

In any case, I think that it is best for this discussion to take place on the 
llvm-dev list where it is likely to get the most visibility.  Would you mind 
moving comments and discussions there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

> 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.

-Chris


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

I am very much +1 on this.  That said, this isn't the sort of thing we just use 
patch review for.  Please agitate a robust discussion about this on llvm-dev.  
:-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits