Re: [PATCH] D22057: Prevent devirtualization of calls to un-instantiated functions.
Prazek added a subscriber: Prazek. Comment at: lib/Sema/Sema.cpp:684 @@ +683,3 @@ + for (auto PII : Pending) +if (FunctionDecl *Func = dyn_cast(PII.first)) + Func->setMarkedForPendingInstantiation(); Dry. Use auto https://reviews.llvm.org/D22057 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24439: [Clang] Fix some Clang-tidy modernize-use-bool-literals and Include What You Use warnings; other minor fixes
Prazek added a subscriber: Prazek. Prazek accepted this revision. Prazek added a reviewer: Prazek. Prazek added a comment. This revision is now accepted and ready to land. Lgtm Repository: rL LLVM https://reviews.llvm.org/D24439 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r281150 - CodeGen: remove unnecessary else case
Author: compnerd
Date: Sat Sep 10 20:25:15 2016
New Revision: 281150
URL: http://llvm.org/viewvc/llvm-project?rev=281150&view=rev
Log:
CodeGen: remove unnecessary else case
Refactor the assignment so that its much more clear that the if-clause contains
the lookup, and once cached is directly used. NFC.
Modified:
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=281150&r1=281149&r2=281150&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Sat Sep 10 20:25:15 2016
@@ -3152,7 +3152,6 @@ CodeGenModule::GetAddrOfConstantCFString
llvm::Constant *Zero = llvm::Constant::getNullValue(Int32Ty);
llvm::Constant *Zeros[] = { Zero, Zero };
- llvm::Value *V;
// If we don't already have it, get __CFConstantStringClassReference.
if (!CFConstantStringClassRef) {
@@ -3182,10 +3181,8 @@ CodeGenModule::GetAddrOfConstantCFString
}
// Decay array -> ptr
-V = llvm::ConstantExpr::getGetElementPtr(Ty, GV, Zeros);
-CFConstantStringClassRef = V;
- } else {
-V = CFConstantStringClassRef;
+CFConstantStringClassRef =
+llvm::ConstantExpr::getGetElementPtr(Ty, GV, Zeros);
}
QualType CFTy = getContext().getCFConstantStringType();
@@ -3195,7 +3192,7 @@ CodeGenModule::GetAddrOfConstantCFString
llvm::Constant *Fields[4];
// Class pointer.
- Fields[0] = cast(V);
+ Fields[0] = cast(CFConstantStringClassRef);
// Flags.
llvm::Type *Ty = getTypes().ConvertType(getContext().UnsignedIntTy);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24383: Add addOrMerge interface to tooling::Replacements.
@ioeric:
1.
regarding B:
i apologize in advance, i am not sure if it's directly related to your
patch, but i'm kinda afraid that the current approach will shadow
the issue (if it's considered to be an issue).
For simplicity let's take a look at the following example:
Point.h:
struct Point {};
a.cpp:
include "Point.h"
b.cpp:
include "Point.h"
clang-rename rename-all -i -old-name Point -new-name Point2 a.cpp b.cpp
Renaming failed in /Users/Alexshap/PlayRename/srcs/./include/Point.h! New
replacement:
/Users/Alexshap/PlayRename/srcs/./include/Point.h: 7:+5:"Point2"
conflicts with existing replacement:
/Users/Alexshap/PlayRename/srcs/./include/Point.h: 7:+5:"Point2"
the thing is that in the past Replacements was a typedef on std::set<...>
(if i am not mistaken)
and insert() ignored duplicates. Now Replacements.add(...) will return an
error (the same replacement has already been added).
2.
(Separate observation, I'd like to take advantage of this discussion and
ask about it)
Replacements Replacements::merge(const Replacements &ReplacesToMerge)
The question is the following: with the current implementation we copy
all the replacements even if the merge is "trivial" or if there are only a
few conflicts.
This looks suboptimal, please, correct me if I'm wrong.
3. Having said that - just curious
- is the fallback on merge (in addOrMergeReplacement ) really necessary in
those examples ?
(I mean in change-namespace and include-fixer)
Thanks,
Alex
On Sat, Sep 10, 2016 at 9:43 PM, Daniel Jasper wrote:
> There are several things I find particularly confusing about this.
> Fundamentally, Replacements refer to a specific version of the code. Say
> you have a Replacements object R and a Replacement r that you want to
> integrate into it. Further assume, that C0 is the initial version of the
> code whereas C1 is the version after applying R. If you *add* r to R, that
> means that r (specifically the offset and length in it) refer to C0. If you
> *merge* it, that means that r refers to C1. Correct replacements can always
> be *merged*, while *adding* might cause conflicts if they overlap with
> existing ones. Thus, a function "add or merge" fundamentally doesn't make
> sense to me. These are fundamentally different concepts.
>
> What this function (and the different implementations we already have) is
> fundamentally trying to do is to have a version of *add* that resolves
> (some) conflicts. It does that by assuming that r is currently referring to
> C0, so it first shifts it and then calls *merge*. The fact that you can
> shortcut if there isn't actually a conflict (i.e. the "addOr" part) is just
> an optimization. I am fine with implementing that optimization, but it
> shouldn't be part of the name.
>
> Now, I don't (yet) have a good name as this has very intricate behavior.
> And I am not sure it is really a good idea to have this magically "resolve"
> all conflicts, because I think there are different classes. I think what we
> want here is to have a way to combine replacements that require a defined
> order, but that don't actually conflict. Lets look at a few cases. Assume R
> contains a single Replacement A and you trying to "addOrMerge" another
> Replacement B.
> 1. A and B are both inserts at the same code location. This seems
> relatively benign and we just want a defined order
> 2. A inserts at offset X and B replaces (X, X+N). Still quite benign, but
> we don't want B to replace anything that A inserted, it should replace the
> original text
> 3. B inserts at offset X and A replaces (X, X+N). Same thing, though
> interesting as we now actually have to switch the order
> 4. A and B actually replace overlapping code ranges. This is really bad
> and I think we should continue to report a conflict instead of doing
> something weird
>
> I'd think that your existing implementation gets #1 right but possibly
> only one of #2/#3. It will also do something very interesting for #4 and I
> really think what we want is to report an error.
>
> On Sat, Sep 10, 2016 at 5:53 PM, Eric Liu wrote:
>
>> This function has been implemented in several places, so I figure it
>> might be useful to have this function in the library. Although I'm not sure
>> whether this should be a class interface or just a standalone helper
>> function.
>>
>> @alexshap: thanks for the explanation. For your point B, I'm not sure
>> about the example, could you elaborate a bit? E.g. which use case to be
>> specific? What are the headers?
>>
>> Generally, the way users of `Replacements` class adding new replacements
>> also matters. For example, if you have multiple insertions at the same
>> offset, you could concatenate them and insert as one single replacement, if
>> you care about performance.
>>
>> For most use cases I've seen when converting from old std::set
>> implementation to the current implementation, performance can be improved
>> by changing the way replacements are added. And the most natural way is
>> often the most ef
Re: [PATCH] D24439: [Clang] Fix some Clang-tidy modernize-use-bool-literals and Include What You Use warnings; other minor fixes
compnerd added inline comments.
Comment at: lib/Basic/SourceManager.cpp:395
@@ -383,2 +394,3 @@
+ for (llvm::DenseMap::iterator
I = FileInfos.begin(), E = FileInfos.end(); I != E; ++I) {
if (I->second) {
Cant this be rewritten as:
for (const auto &I : FileInfos)
Comment at: lib/Basic/SourceManager.cpp:1039
@@ -1029,3 +1038,3 @@
bool Invalid = false;
- const SrcMgr::ExpansionInfo &ExpInfo =
+ const ExpansionInfo &ExpInfo =
getSLocEntry(DecompLoc.first, &Invalid).getExpansion();
Hmm, might be nice to change this to `Expansion` as the other ones are named
that.
Comment at: lib/Basic/SourceManager.cpp:1075
@@ -1065,4 +1074,3 @@
bool Invalid = false;
- const SrcMgr::ExpansionInfo &ExpInfo =
- getSLocEntry(FID, &Invalid).getExpansion();
+ const ExpansionInfo &ExpInfo = getSLocEntry(FID, &Invalid).getExpansion();
if (Invalid)
Similar.
Repository:
rL LLVM
https://reviews.llvm.org/D24439
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24383: Add addOrMerge interface to tooling::Replacements.
There are several things I find particularly confusing about this.
Fundamentally, Replacements refer to a specific version of the code. Say
you have a Replacements object R and a Replacement r that you want to
integrate into it. Further assume, that C0 is the initial version of the
code whereas C1 is the version after applying R. If you *add* r to R, that
means that r (specifically the offset and length in it) refer to C0. If you
*merge* it, that means that r refers to C1. Correct replacements can always
be *merged*, while *adding* might cause conflicts if they overlap with
existing ones. Thus, a function "add or merge" fundamentally doesn't make
sense to me. These are fundamentally different concepts.
What this function (and the different implementations we already have) is
fundamentally trying to do is to have a version of *add* that resolves
(some) conflicts. It does that by assuming that r is currently referring to
C0, so it first shifts it and then calls *merge*. The fact that you can
shortcut if there isn't actually a conflict (i.e. the "addOr" part) is just
an optimization. I am fine with implementing that optimization, but it
shouldn't be part of the name.
Now, I don't (yet) have a good name as this has very intricate behavior.
And I am not sure it is really a good idea to have this magically "resolve"
all conflicts, because I think there are different classes. I think what we
want here is to have a way to combine replacements that require a defined
order, but that don't actually conflict. Lets look at a few cases. Assume R
contains a single Replacement A and you trying to "addOrMerge" another
Replacement B.
1. A and B are both inserts at the same code location. This seems
relatively benign and we just want a defined order
2. A inserts at offset X and B replaces (X, X+N). Still quite benign, but
we don't want B to replace anything that A inserted, it should replace the
original text
3. B inserts at offset X and A replaces (X, X+N). Same thing, though
interesting as we now actually have to switch the order
4. A and B actually replace overlapping code ranges. This is really bad and
I think we should continue to report a conflict instead of doing something
weird
I'd think that your existing implementation gets #1 right but possibly only
one of #2/#3. It will also do something very interesting for #4 and I
really think what we want is to report an error.
On Sat, Sep 10, 2016 at 5:53 PM, Eric Liu wrote:
> This function has been implemented in several places, so I figure it might
> be useful to have this function in the library. Although I'm not sure
> whether this should be a class interface or just a standalone helper
> function.
>
> @alexshap: thanks for the explanation. For your point B, I'm not sure
> about the example, could you elaborate a bit? E.g. which use case to be
> specific? What are the headers?
>
> Generally, the way users of `Replacements` class adding new replacements
> also matters. For example, if you have multiple insertions at the same
> offset, you could concatenate them and insert as one single replacement, if
> you care about performance.
>
> For most use cases I've seen when converting from old std::set
> implementation to the current implementation, performance can be improved
> by changing the way replacements are added. And the most natural way is
> often the most efficient one.
>
>
> On Sat, Sep 10, 2016, 10:54 Alexander Shaposhnikov
> wrote:
>
>> alexshap added a comment.
>>
>> disclaimer: i don't know if this method is the right thing to add (to be
>> honest i would prefer a better interface but don't have any good
>> suggestions on my mind at the moment), i see several issues (IMO, i might
>> be mistaken, apologize in advance) with the current interface of
>> Replacements. I will not list all of them, but want to add some context:
>>
>> A. right now i see the same code (with minor changes) in several places:
>>
>> 1.
>>
>> llvm/tools/clang/tools/extra/include-fixer/IncludeFixer.cpp +382
>>
>> auto R = tooling::Replacement(
>> {FilePath, Info.Range.getOffset(), Info.Range.getLength(),
>> Context.getHeaderInfos().front().QualifiedName});
>>auto Err = Replaces.add(R);
>>if (Err) {
>> llvm::consumeError(std::move(Err));
>> R = tooling::Replacement(
>> R.getFilePath(), Replaces.getShiftedCodePosition(R.getOffset()),
>> R.getLength(), R.getReplacementText());
>> Replaces = Replaces.merge(tooling::Replacements(R));
>>}
>>
>> 2.
>>
>> llvm/tools/clang/tools/extra/change-namespace/ChangeNamespace.cpp +126
>> (see https://reviews.llvm.org/D24183)
>>
>>
>> void addOrMergeReplacement(const tooling::Replacement &R,
>>tooling::Replacements *Replaces) {
>>auto Err = Replaces->add(R);
>>if (Err) {
>> llvm::consumeError(std::move(Err));
>> auto Replace = getReplacementInChangedCode(*Replaces, R);
>> *Replaces = Replaces->merge(tooling::Replacements(Replace));
>>
Re: [PATCH] D24383: Add addOrMerge interface to tooling::Replacements.
This function has been implemented in several places, so I figure it might
be useful to have this function in the library. Although I'm not sure
whether this should be a class interface or just a standalone helper
function.
@alexshap: thanks for the explanation. For your point B, I'm not sure
about the example, could you elaborate a bit? E.g. which use case to be
specific? What are the headers?
Generally, the way users of `Replacements` class adding new replacements
also matters. For example, if you have multiple insertions at the same
offset, you could concatenate them and insert as one single replacement, if
you care about performance.
For most use cases I've seen when converting from old std::set
implementation to the current implementation, performance can be improved
by changing the way replacements are added. And the most natural way is
often the most efficient one.
On Sat, Sep 10, 2016, 10:54 Alexander Shaposhnikov
wrote:
> alexshap added a comment.
>
> disclaimer: i don't know if this method is the right thing to add (to be
> honest i would prefer a better interface but don't have any good
> suggestions on my mind at the moment), i see several issues (IMO, i might
> be mistaken, apologize in advance) with the current interface of
> Replacements. I will not list all of them, but want to add some context:
>
> A. right now i see the same code (with minor changes) in several places:
>
> 1.
>
> llvm/tools/clang/tools/extra/include-fixer/IncludeFixer.cpp +382
>
> auto R = tooling::Replacement(
> {FilePath, Info.Range.getOffset(), Info.Range.getLength(),
> Context.getHeaderInfos().front().QualifiedName});
>auto Err = Replaces.add(R);
>if (Err) {
> llvm::consumeError(std::move(Err));
> R = tooling::Replacement(
> R.getFilePath(), Replaces.getShiftedCodePosition(R.getOffset()),
> R.getLength(), R.getReplacementText());
> Replaces = Replaces.merge(tooling::Replacements(R));
>}
>
> 2.
>
> llvm/tools/clang/tools/extra/change-namespace/ChangeNamespace.cpp +126
> (see https://reviews.llvm.org/D24183)
>
>
> void addOrMergeReplacement(const tooling::Replacement &R,
>tooling::Replacements *Replaces) {
>auto Err = Replaces->add(R);
>if (Err) {
> llvm::consumeError(std::move(Err));
> auto Replace = getReplacementInChangedCode(*Replaces, R);
> *Replaces = Replaces->merge(tooling::Replacements(Replace));
>}
> }
>
> B. For replacements in headers we can get into if(Err) branch quite often
> because the same replacement can be generated multiple times (if that
> header is included into several *.cpp files).
>
>
> https://reviews.llvm.org/D24383
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21113: Add support for case-insensitive header lookup
The current status is that this is too slow, so for now the recommendation is to have a case insensitive mount for the files that need it. On Sep 9, 2016 8:18 PM, "John Sheu" wrote: > sheu added a subscriber: sheu. > sheu added a comment. > > I'd be very interested in seeing this patch happen. What's the current > status here? > > Also, w.r.t. the cache invalidation problem -- would it be feasible / a > good idea to move users of the FileSystem API to VirtualFileSystem, in > general? This would have the nice effect of putting filesystem things all > in once place. Then invalidation tracking etc. will be easier. > > > https://reviews.llvm.org/D21113 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r281136 - Add missing test coverage for an inheritance model attrib merge diag.
Author: nico
Date: Sat Sep 10 08:03:59 2016
New Revision: 281136
URL: http://llvm.org/viewvc/llvm-project?rev=281136&view=rev
Log:
Add missing test coverage for an inheritance model attrib merge diag.
Without this, no tests fail if I remove the Diag() in the first if in
Sema::mergeMSInheritanceAttr().
Modified:
cfe/trunk/test/SemaCXX/member-pointer-ms.cpp
Modified: cfe/trunk/test/SemaCXX/member-pointer-ms.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/member-pointer-ms.cpp?rev=281136&r1=281135&r2=281136&view=diff
==
--- cfe/trunk/test/SemaCXX/member-pointer-ms.cpp (original)
+++ cfe/trunk/test/SemaCXX/member-pointer-ms.cpp Sat Sep 10 08:03:59 2016
@@ -291,3 +291,11 @@ static_assert(sizeof(int SingleInheritan
#pragma pointers_to_members(single) // expected-error{{unexpected 'single'}}
#endif
+
+namespace merging {
+struct __single_inheritance S;
+struct __single_inheritance S;
+
+struct __single_inheritance M; // expected-note{{previous inheritance model
specified here}}
+struct __multiple_inheritance M; // expected-error{{inheritance model does not
match previous declaration}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22130: Link static PIE programs against rcrt0.o on OpenBSD
Ed Maste wrote: > emaste added a comment. > > Seems fine to me, but I'm not particularly knowledgeable about OpenBSD's > toolchain. Could you commit it please if it looks ok? This diff is what OpenBSD has in its tree. We'd like to get it upstream. > https://reviews.llvm.org/D22130 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24395: Align declarations that are preceded by different number of commas.
djasper added a comment. I think this will also be solved by https://reviews.llvm.org/D21279. https://reviews.llvm.org/D24395 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24383: Add addOrMerge interface to tooling::Replacements.
alexshap added a comment.
disclaimer: i don't know if this method is the right thing to add (to be honest
i would prefer a better interface but don't have any good suggestions on my
mind at the moment), i see several issues (IMO, i might be mistaken, apologize
in advance) with the current interface of Replacements. I will not list all of
them, but want to add some context:
A. right now i see the same code (with minor changes) in several places:
1.
llvm/tools/clang/tools/extra/include-fixer/IncludeFixer.cpp +382
auto R = tooling::Replacement(
{FilePath, Info.Range.getOffset(), Info.Range.getLength(),
Context.getHeaderInfos().front().QualifiedName});
auto Err = Replaces.add(R);
if (Err) {
llvm::consumeError(std::move(Err));
R = tooling::Replacement(
R.getFilePath(), Replaces.getShiftedCodePosition(R.getOffset()),
R.getLength(), R.getReplacementText());
Replaces = Replaces.merge(tooling::Replacements(R));
}
2.
llvm/tools/clang/tools/extra/change-namespace/ChangeNamespace.cpp +126 (see
https://reviews.llvm.org/D24183)
void addOrMergeReplacement(const tooling::Replacement &R,
tooling::Replacements *Replaces) {
auto Err = Replaces->add(R);
if (Err) {
llvm::consumeError(std::move(Err));
auto Replace = getReplacementInChangedCode(*Replaces, R);
*Replaces = Replaces->merge(tooling::Replacements(Replace));
}
}
B. For replacements in headers we can get into if(Err) branch quite often
because the same replacement can be generated multiple times (if that header is
included into several *.cpp files).
https://reviews.llvm.org/D24383
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes
djasper added inline comments.
Comment at: lib/Format/WhitespaceManager.cpp:47
@@ +46,3 @@
+
+int WhitespaceManager::Change::ScopeLevel() const {
+ // NestingLevel is raised on the opening paren/square, and remains raised
What I don't understand is why you have to combine NestingLevel and IndentLevel
at all. To me it feels wrong to add them no matter what (with and without your
extra bit of logic).
IMO, for alignment, we should ensure that both NestingLevel *and* IndentLevel
are the same, not just the the sum of the two is the same. That's why I was
suggesting putting them into a pair and just comparing the pair. But I might be
missing something very obvious.
https://reviews.llvm.org/D21279
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21026: [clang-format] append newline after code when inserting new headers at the end of the code which does not end with newline.
djasper added a comment.
Does this still work with the new Replacements?
Comment at: lib/Format/Format.cpp:1572
@@ +1571,3 @@
+// When inserting headers at end of the code, also insert a '\n' at the end
+// of the code if it does not ends with '\n'.
+// The way of inserting '\n' is a bit of hack since we have no control over
"does not end"
Comment at: lib/Format/Format.cpp:1574
@@ +1573,3 @@
+// The way of inserting '\n' is a bit of hack since we have no control over
+// wether header insertions with the same offset (i.e. `Code.size()`) are
+// inserted after the new line insertion ('\n' would actually be inserted
"whether"
https://reviews.llvm.org/D21026
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24401: clang-format: Add Java detection to git-clang-format.
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. Repository: rL LLVM https://reviews.llvm.org/D24401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18313: clang-format: Make include sorting's main include detection configurable.
djasper closed this revision. djasper marked 6 inline comments as done. djasper added a comment. Submitted as r263943. Comment at: include/clang/Format/Format.h:415 @@ +414,3 @@ + /// as the "main" include in both a.cc and a_test.cc. + std::string IncludeMainRegex; + klimek wrote: > djasper wrote: > > I chose this name for better alphabetical ordering. I don't strongly lean > > either way, WDYT? > I'd still lean slightly towards MainIncludeRegex... but feel free to keep. Renamed to IncludeIsMainRegex, which seems to be better than either of the other. https://reviews.llvm.org/D18313 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24383: Add addOrMerge interface to tooling::Replacements.
djasper added a comment. This seems like a pretty weird function to have in the interface. Could you explain what use case(s) you are envisioning? https://reviews.llvm.org/D24383 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
