JDevlieghere added a comment.
Thanks for the reviews everyone!
I'm currently still stuck on an issue I discovered when processing LLVM. I
asked for help on the mailing list [0] but maybe someone here can help.
Basically the issue is that sometimes pointers are passed to memcpy for which
the ty
Prazek added a comment.
In https://reviews.llvm.org/D22725#495329, @etienneb wrote:
> In https://reviews.llvm.org/D22725#494167, @Prazek wrote:
>
> > hmm It seems that I mislead you, I suck at C api - memmove source and
> > destination can overlap, but std::move can't. So I guess you have to rem
etienneb added inline comments.
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:73
@@ +72,3 @@
+ Finder->addMatcher(
+ callExpr(allOf(callee(functionDecl(matchesName("::memcpy"))),
+ argumentCountIs(3)))
It's more efficient to use
etienneb added a comment.
In https://reviews.llvm.org/D22725#494167, @Prazek wrote:
> hmm It seems that I mislead you, I suck at C api - memmove source and
> destination can overlap, but std::move can't. So I guess you have to remove
> the memmove support. Sorry.
On windows (MSVC CRT), both f
Prazek added a comment.
In https://reviews.llvm.org/D22725#494168, @JDevlieghere wrote:
> In https://reviews.llvm.org/D22725#494167, @Prazek wrote:
>
> > hmm It seems that I mislead you, I suck at C api - memmove source and
> > destination can overlap, but std::move can't. So I guess you have to
JDevlieghere added a comment.
In https://reviews.llvm.org/D22725#494167, @Prazek wrote:
> hmm It seems that I mislead you, I suck at C api - memmove source and
> destination can overlap, but std::move can't. So I guess you have to remove
> the memmove support. Sorry.
No problem, I wasn't awar
Prazek added a comment.
hmm It seems that I mislead you, I suck at C api - memmove source and
destination can overlap, but std::move can't. So I guess you have to remove the
memmove support. Sorry.
https://reviews.llvm.org/D22725
___
cfe-commits m
JDevlieghere added a comment.
In https://reviews.llvm.org/D22725#494074, @Prazek wrote:
> Maybe the right way would be to have check called
> 'modernize-use-sequence-algorithm' or just 'modernize-use-algorithm' that
> would basically do all those stuff. It would be good to not introduce 5 new
JDevlieghere removed rL LLVM as the repository for this revision.
JDevlieghere updated this revision to Diff 65273.
JDevlieghere added a comment.
- Extended check to replace memmove with std::move and memset with std::fill
- Renamed check to modernize-use-algorithm (I'd be equally fine with moving
Prazek added a comment.
In https://reviews.llvm.org/D22725#493970, @JDevlieghere wrote:
> In https://reviews.llvm.org/D22725#493947, @Prazek wrote:
>
> > Thanks for the contribution. Is it your first check?
>
>
> Yes, it is! :-)
Cool! hope to see some other cool checks. You could probably use p
Prazek added inline comments.
Comment at: clang-tidy/modernize/UseCopyCheck.cpp:49
@@ +48,3 @@
+ if (getLangOpts().CPlusPlus) {
+Inserter.reset(new utils::IncludeInserter(
+Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));
there is
On Sat, Jul 23, 2016 at 4:16 PM, Aaron Ballman wrote:
> On Sat, Jul 23, 2016 at 1:38 PM, Piotr Padlewski
> wrote:
>> Prazek added a subscriber: Prazek.
>> Prazek added a comment.
>>
>> Thanks for the contribution. Is it your first check?
>>
>> Some main issues:
>>
>> 1. I think it would be much b
On Sat, Jul 23, 2016 at 1:38 PM, Piotr Padlewski
wrote:
> Prazek added a subscriber: Prazek.
> Prazek added a comment.
>
> Thanks for the contribution. Is it your first check?
>
> Some main issues:
>
> 1. I think it would be much better to move this check to modernize module. I
> think the name '
Eugene.Zelenko added inline comments.
Comment at: docs/ReleaseNotes.rst:70
@@ +69,3 @@
+
+ Replaces calls to memcpy with std::copy.
+
Please highlight memcpy and std::copy with `` as it done in documentation.
Repository:
rL LLVM
https://reviews.llvm.org/D227
JDevlieghere added a comment.
In https://reviews.llvm.org/D22725#493947, @Prazek wrote:
> Thanks for the contribution. Is it your first check?
Yes, it is! :-)
> Some main issues:
>
> 1. I think it would be much better to move this check to modernize module. I
> think the name 'modernize-us
JDevlieghere updated this revision to Diff 65246.
JDevlieghere added a comment.
- Added new check to release notes
- Renamed to modernize-use-copy
- Addressed comments from Prazek
Repository:
rL LLVM
https://reviews.llvm.org/D22725
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/mo
Prazek added a comment.
In https://reviews.llvm.org/D22725#493942, @JDevlieghere wrote:
> In https://reviews.llvm.org/D22725#493940, @aaron.ballman wrote:
>
> > > Using std::copy opens up the possibility of type-aware optimizations
> > > which are not possible with memcpy.
> >
> >
> > To my know
aaron.ballman added a comment.
In https://reviews.llvm.org/D22725#493942, @JDevlieghere wrote:
> In https://reviews.llvm.org/D22725#493940, @aaron.ballman wrote:
>
> > > Using std::copy opens up the possibility of type-aware optimizations
> > > which are not possible with memcpy.
> >
> >
> > To
Prazek added a subscriber: Prazek.
Prazek added a comment.
Thanks for the contribution. Is it your first check?
Some main issues:
1. I think it would be much better to move this check to modernize module. I
think the name 'modernize-use-copy' would follow the convention, or just
'modernize-rep
JDevlieghere added a comment.
In https://reviews.llvm.org/D22725#493940, @aaron.ballman wrote:
> > Using std::copy opens up the possibility of type-aware optimizations which
> > are not possible with memcpy.
>
>
> To my knowledge, those type-aware optimizations are for transforming copies
> inv
aaron.ballman added a comment.
> Using std::copy opens up the possibility of type-aware optimizations which
> are not possible with memcpy.
To my knowledge, those type-aware optimizations are for transforming copies
involving trivially-copyable types into calls to memcpy(). What other
optimiz
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).
May be modernize will be better place then misc?
https://reviews.llvm.org/D22725
___
cfe-
22 matches
Mail list logo
