mehdi_amini added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2376
+const VarDecl *InitDecl;
+const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+if (InitExpr) {
ahatanak wrote:
> Does getAnyInitializer ever return a null poin
mehdi_amini added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2374
+ llvm::Constant *Init = nullptr;
+ if (D && D->isConstexpr() && !D->isInline() && !D->hasAttr())
{
+const VarDecl *InitDecl;
rsmith wrote:
> Applying this for only `co
mehdi_amini added inline comments.
Comment at: src/cxa_demangle.cpp:44
+class string_ref
+{
If this is supposed to be *the* ultimate LLVM demangler, can we follow LLVM
coding standard?
https://reviews.llvm.org/D35159
__
mehdi_amini added inline comments.
Comment at: src/cxa_demangle.cpp:87
+
+class stream
+{
Doc
(same for non trivial APIs)
Comment at: src/cxa_demangle.cpp:125
+
+typedef unsigned stream_position;
+
Doc
Co
mehdi_amini updated this revision to Diff 105787.
mehdi_amini added a comment.
Fix issues around mutable fields and regression on "internal", add more testing.
https://reviews.llvm.org/D34992
Files:
clang/lib/CodeGen/CodeGenModule.cpp
clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
Index:
mehdi_amini added inline comments.
Comment at: cfe/trunk/lib/CodeGen/CodeGenModule.cpp:3759
+ unsigned AddrSpace =
+ VD ? GetGlobalVarAddressSpace(VD) : MaterializedType.getAddressSpace();
+ auto TargetAS = getContext().getTargetAddressSpace(AddrSpace);
Co
mehdi_amini added inline comments.
Comment at: src/cxa_demangle.cpp:44
+class string_ref
+{
dexonsmith wrote:
> erik.pilkington wrote:
> > mehdi_amini wrote:
> > > If this is supposed to be *the* ultimate LLVM demangler, can we follow
> > > LLVM coding standar
mehdi_amini added a comment.
In https://reviews.llvm.org/D35159#807075, @dexonsmith wrote:
> This LGTM. Mehdi, do you have any other concerns?
No other concern (haven't looked further for any either)
https://reviews.llvm.org/D35159
___
cfe-commi
mehdi_amini added a comment.
In https://reviews.llvm.org/D35081#807172, @fhahn wrote:
> It's @yunlian, so if the issue you described above covers his failure, than
> that's great. @tejohnson do you have time to work on a fix soon? Otherwise I
> could give it a try, I would probably need a few p
mehdi_amini added a comment.
Teresa: can you describe a bit more how we end up importing two summaries for
the same GUID? I would expect that after importing one, we don't even come to
try to import another since we already have one.
https://reviews.llvm.org/D35081
_
mehdi_amini added a comment.
Thanks, very clear :)
I would expect that if we reprocess a GUID we invalidate the previous import of
the same GUID. Right now my impression is that the issue is that `
ImportList[ExportModulePath][VI.getGUID()]` is indexed on the importing module.
So it'd require
mehdi_amini added a comment.
In https://reviews.llvm.org/D35081#808189, @tejohnson wrote:
> > Alternatively (and likely preferably from a compile-time point of view to
> > limit the list of import), we could keep a map of GUID->Summary and reuse
> > it before trying to select a new callee.
>
>
mehdi_amini added a comment.
In https://reviews.llvm.org/D35081#808207, @tejohnson wrote:
> My first option was if any copy is under the threshold, simply pick the
> prevailing copy (it may be over threshold, but assume we want that one
> anyway). Another possibility is to only allow importing
mehdi_amini added a comment.
@rsmith: post-C++-commitee-meeting ping ;)
https://reviews.llvm.org/D34992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mehdi_amini added a comment.
Here is the current output:
clang version 5.0.0
Target: x86_64-apple-darwin16.6.0
Thread model: posix
InstalledDir: /Users/mamini/projects/llvm/./clangDebug/bin
Registered Targets:
aarch64- AArch64 (little endian)
aarch64_be - AArch64 (
mehdi_amini added a comment.
Weekly ping! (@rsmith)
https://reviews.llvm.org/D34992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mehdi_amini added a comment.
In https://reviews.llvm.org/D33900#820281, @hans wrote:
> In https://reviews.llvm.org/D33900#818968, @mehdi_amini wrote:
>
> > I think @thakis is right: this too verbose to be the default --version.
> > We likely shouldn't ship this in clang-5.0 (@hans).
>
>
> Let me
This revision was automatically updated to reflect the committed changes.
Closed by commit rL294696: Fully qualify (preprend ::) calls to math functions
from libc (authored by mehdi_amini).
Changed prior to commit:
https://reviews.llvm.org/D29804?vs=87933&id=87939#toc
Repository:
rL LLVM
ht
mehdi_amini added a comment.
Ping :)
https://reviews.llvm.org/D28404
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mehdi_amini added a comment.
Also note that @chandlerc in r290398 made clang adding "noinline" on every
function at O0 by default, which seems very similar to what I'm doing here.
We're still waiting for @rsmith to comment whether it'd be better to `have a
LangOpts flag that basically means "pr
mehdi_amini added a comment.
In https://reviews.llvm.org/D13330#582607, @rsmith wrote:
> I think this attribute is poorly named. Explicit instantiation definitions
> are *already* required to be globally unique; see [temp.spec]/5.1:
>
> "For a given template and a given set of template-arguments
mehdi_amini added a comment.
In https://reviews.llvm.org/D30239#683116, @pcc wrote:
> Can you please add a ThinLTO test to lld before adding driver support?
Is clang-cl using lld as default? How is the switch done? Ideally we should
have a nice error message from the driver if -flto is used wi
mehdi_amini added a comment.
In https://reviews.llvm.org/D30372#687060, @ahatanak wrote:
> In https://reviews.llvm.org/D30372#687054, @dlj wrote:
>
> > In https://reviews.llvm.org/D30372#686871, @ahatanak wrote:
> >
> > > Have you considered using "include_directories" in CMakeLists.txt to
> > >
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
(It seems that having libc++ and libc++abi in the same repo would help sharing
code like this)
https://reviews.llvm.org/D30514
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D30516
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/list
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
Comment at: CMakeLists.txt:416
+set(LIBCXXABI_HAS_UNDEFINED_SYMBOLS ((NOT
LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS)
+OR (LIBCXXABI_BUILD_EXTERNAL_THREAD_L
mehdi_amini added a comment.
In https://reviews.llvm.org/D30514#690702, @jroelofs wrote:
> In https://reviews.llvm.org/D30514#690318, @mehdi_amini wrote:
>
> > LGTM.
> >
> > (It seems that having libc++ and libc++abi in the same repo would help
> > sharing code like this)
>
>
> I think it would
mehdi_amini added a comment.
In https://reviews.llvm.org/D30239#691972, @rnk wrote:
> Do you guys think that maybe -flto should imply -fuse-ld=lld, or is that too
> much magic?
I don't know enough about windows, but I would think it may be surprising for a
user that -flto changes the linker b
mehdi_amini updated this revision to Diff 90773.
mehdi_amini edited the summary of this revision.
mehdi_amini added a comment.
Rebase
https://reviews.llvm.org/D17469
Files:
test/std/localization/locale.categories/category.ctype/ctype_base.pass.cpp
test/std/localization/locale.categories/ca
mehdi_amini updated this revision to Diff 90779.
mehdi_amini added a comment.
Add back the python changes (files were moved, I lost them in the rebase)
https://reviews.llvm.org/D17469
Files:
test/std/localization/locale.categories/category.ctype/ctype_base.pass.cpp
test/std/localization/lo
mehdi_amini added inline comments.
Comment at: test/libcxx/test/config.py:66
self.env = {}
self.use_target = False
self.use_system_cxx_lib = False
EricWF wrote:
> Was the omission of `self.use_deployment` here intentional? Python linter
mehdi_amini added inline comments.
Comment at: test/libcxx/test/config.py:66
self.env = {}
self.use_target = False
self.use_system_cxx_lib = False
mehdi_amini wrote:
> EricWF wrote:
> > Was the omission of `self.use_deployment` here int
mehdi_amini created this revision.
Both libc++ and libc++abi export a weak definition of operator
new/delete. On Darwin, this can often cause dirty __DATA in the
shared cache when having to switch from one to the other. Instead,
libc++ should reexport libc++abi's implementation of these symbols.
mehdi_amini added a comment.
Note: this is already shipping like this for ~2 years in our OSes.
https://reviews.llvm.org/D30765
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mehdi_amini added a comment.
Off topic, but since this is a rev lock change with LLVM, you can to all of in
a single revision with:
http://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo
Repository:
rL LLVM
https://reviews.llvm.org/D30792
mehdi_amini added a comment.
Agree with @pcc. Unless anyone has a need to have "perfect" support for Os,
this is the right direction.
https://reviews.llvm.org/D30920
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-
mehdi_amini added a comment.
In https://reviews.llvm.org/D30920#700433, @tejohnson wrote:
> In https://reviews.llvm.org/D30920#700133, @pcc wrote:
>
> > In https://reviews.llvm.org/D30920#700077, @tejohnson wrote:
> >
> > > Until everything is converted to using size attributes, it seems like a
mehdi_amini added a comment.
What is the justification for a platform specific default change here?
Repository:
rL LLVM
https://reviews.llvm.org/D27163
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/
mehdi_amini added a comment.
In https://reviews.llvm.org/D27163#606744, @arphaman wrote:
> In https://reviews.llvm.org/D27163#606695, @mehdi_amini wrote:
>
> > What is the justification for a platform specific default change here?
>
>
> The flag itself is platform agnostic, however, the default v
mehdi_amini added inline comments.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:530
+ // Don't need to mark Objective-C methods or blocks since the undefined
+ // behaviour optimization isn't used for them.
+}
Quuxplusone wrote:
> This seems like a trap waiti
mehdi_amini added inline comments.
Comment at: test/CodeGenCXX/return.cpp:21
+ // CHECK-NOSTRICT-NEXT: load
+ // CHECK-NOSTRICT-NEXT: ret i32
+ // CHECK-NOSTRICT-NEXT: }
rjmccall wrote:
> mehdi_amini wrote:
> > Quuxplusone wrote:
> > > Can you explain why a lo
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
Repository:
rL LLVM
https://reviews.llvm.org/D27298
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
https://reviews.llvm.org/D27332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/lis
mehdi_amini added a comment.
(You have a typo in the decription, you may want to fix it before commit)
https://reviews.llvm.org/D27332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mehdi_amini added a comment.
In https://reviews.llvm.org/D26376#597614, @mclow.lists wrote:
> More info - The following code:
>
> #include
> int main () {}
>
>
> fails to compile on either gcc 6.2 (locally), gcc 7 head (online compiler) or
> MSVC (online compiler).
Interesting, that le
mehdi_amini abandoned this revision.
mehdi_amini added a comment.
Thanks for the information! Closing this.
https://reviews.llvm.org/D26376
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co
mehdi_amini added inline comments.
Comment at: include/clang/Basic/DiagnosticDriverKinds.td:95
def err_drv_force_crash : Error<
- "failing because environment variable '%0' is set">;
+ "failing because %select{environment variable|option}0 '%1' is set">;
def err_drv_invalid_m
mehdi_amini marked 5 inline comments as done.
mehdi_amini added inline comments.
Comment at: utils/libcxx/test/config.py:289
+def configure_availability(self):
+# FIXME doc
+self.with_availability = self.get_lit_bool('with_availability', False)
---
mehdi_amini added a comment.
@mclow.lists you mentioned on IRC you may have some suggestions to make this
less noisy? Do you still plan to review?
https://reviews.llvm.org/D31739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
mehdi_amini added a comment.
@mclow.lists you mentioned on IRC you may have some suggestions to make this
less noisy? Do you still plan to review?
https://reviews.llvm.org/D31739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
mehdi_amini added a comment.
> To not break LTO with different optimizations levels, we should insert the
> barrier regardles of optimization level.
What do you mean by "break"? Is it a correctness issue?
https://reviews.llvm.org/D32401
___
cfe-co
mehdi_amini added a comment.
I tend to agree with @rjmccall on the principle. Howerever:
> The optimization design seems to rely on anticipating every case that should
> disable the optimization, hence this patch adding special-case logic to the
> frontend, and the 3 other patch
I believe this
mehdi_amini added a comment.
Something still isn't clear to me: the issue IIUC is not with the
`unwindHeader` itself, but with the misaligned thrown object that is allocated
right after the __cxa_exception itself. Isn't it?
Then are you aligning the `unwindHeader` field and by side-effect the s
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D33030#751723, @ahatanak wrote:
> If field unwindHeader is annotated with the aligned attribute, both the field
> and struct __cxa_exception are aligned.
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
https://reviews.llvm.org/D33177
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/lis
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
https://reviews.llvm.org/D32401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/lis
mehdi_amini added a comment.
I'm not sure how it'd help clients of the released version of LLVM to delay,
the development branch and the release seems fairly disconnected to me from
this point of view.
(what can help are deprecating APIs with instructions about how to update so
that they can up
mehdi_amini added a comment.
Please expand the description and include how you figured this out and how do
we know it is actually correct.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120375/new/
https://reviews.llvm.org/D120375
mehdi_amini added a comment.
What matter isn't "convergent" in itself, it is how it restricts
transformations: if you know that all the control flow is always uniform, are
there still restriction on any transformation in presence of convergent
instructions?
If not, then the "target approach" s
mehdi_amini created this revision.
mehdi_amini added reviewers: rriddle, jpienaar, Mogball.
Herald added a subscriber: carlosgalvezp.
mehdi_amini requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
This option allows to eliminate pa
mehdi_amini added inline comments.
Comment at:
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:25
+
+ void a(int ) { /*some code that doesn't use `i`*/ }
+
jpienaar wrote:
> OOC why the extra space after the type?
The space is in the origina
mehdi_amini created this revision.
mehdi_amini added a reviewer: Eugene.Zelenko.
Herald added subscribers: carlosgalvezp, xazax.hun.
mehdi_amini requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
The rational to avoid applying the
mehdi_amini created this revision.
mehdi_amini added a reviewer: Eugene.Zelenko.
Herald added subscribers: carlosgalvezp, xazax.hun.
mehdi_amini requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
These weren't tracked and so weren'
mehdi_amini added inline comments.
Comment at:
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:43
+ a human reader, and there's basically no place for a bug to hide. On the
other
+ hand for non-public functions, all the call-sites are visible and the
pa
mehdi_amini added a comment.
Thanks @aaron.ballman for the review!
Comment at:
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp:157-159
// CHECK-FIXES: C() {}
C(int i) {}
// CHECK-MESSAGES: :[[@LINE-1]]:9: warning
aaron.ballman wrote:
mehdi_amini abandoned this revision.
mehdi_amini added inline comments.
Comment at:
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:43
+ a human reader, and there's basically no place for a bug to hide. On the
other
+ hand for non-public functions, all t
mehdi_amini added a comment.
> @goncharov @mehdi_amini is there a way to change the way the phab UI or the
> pre-merge checks work here to make failing pre-merge checks clearer? This
> isn't the first time I've seen someone be very confused about this UI
I asked the same thing a few days ago an
301 - 367 of 367 matches
Mail list logo
