Hello guys, I agree with all your comments. So I fixed the initial patch. See attach. Best regards, Alexey.
----- Original Message ----- From: Richard Smith Sent: 12/20/12 01:12 AM To: Dmitri Gribenko Subject: Re: [cfe-commits] [PATCH] First OpenMP patch On Wed, Dec 19, 2012 at 1:09 PM, Dmitri Gribenko <[email protected]> wrote: > On Wed, Dec 19, 2012 at 10:53 PM, Douglas Gregor <[email protected]> wrote: >> >> On Dec 19, 2012, at 3:30 AM, Dmitri Gribenko <[email protected]> wrote: >> >>> On Wed, Dec 19, 2012 at 1:02 PM, Alexey Bataev <[email protected]> wrote: >>>> Dmitry, thanks for a good advice about :option:. Fixed patch is in the >>>> attach. >>> >>> LGTM. I think that there is a consensus about command line flags and >>> warnings, so this can go in. >> >> Sorry to come late to the party, but I think this is the wrong approach for our users. Our current behavior is that we simply drop "-fopenmp". If we change that behavior to >> >> warning: Experimental OpenMP support enabled >> >> how have we helped the user? At best, it does nothing and they ignore the warning. At worst, it opens up a ton of questions: should I depend on the experimental implementation? What does it actually do? What was it doing before? How do I! shut this warning off? This is especially important to consider for options like -fopenmp that currently exist for GNU compatibility. Changing the behavior in any way other than actually implementing that functionality is not a net win for users. >> >> All we need for OpenMP at this stage is a new LangOpt for OpenMP (which is part of the patch) and a -cc1 option "-fopenmp" that turns on that LangOpt. It also makes sense to define _OPENMP when that LangOpt is true, so that we can write a test for -fopenmp. >> >> It is very important that "-fopenmp" still be dropped by the driver. The -fopenmp at the -cc1 level will be separate, and used for our testing. Once OpenMP is actually usable, we can wire the driver's -fopenmp up to the -cc1 -fopenmp. > > That's a good point. But in order to test the implementation on some > real OpenMP programs easily, we might still want to introduce a > frontend option, just a different one -- like '-fexperimental-openmp'. > We can also drop the ! warning then. For testing, we can use -Xclang -fopenmp
patch.p
Description: Attachment: patch.p
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
