Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer
On 08/29/2017 10:19 AM, Emil Velikov wrote: > On 29 August 2017 at 18:10, Matt Turner wrote: >> On Tue, Aug 29, 2017 at 3:35 AM, Emil Velikov >> wrote: >>> On 29 August 2017 at 11:11, Eric Engestrom >>> wrote: On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote: > Avoids Clang's warning about the current code: > >warning: suggest braces around initialization of subobject I'm not sure about this patch [1], but everything else in this series is: >>> IIRC "{}" is the least likely way to avoid the warnings, across >>> GCC/Clang versions. >>> It's not part of the C spec, but since it works - not sure how much to >>> bother. >> >> I'm surprised to hear you say that. I've tested with gcc-4.9.4, 6.4.0, >> 7.1.0; and clang-4.0, and none of them warn about {} >> > Eric corrected me on IRC, but I forgot to mention here - "s/least/most/" > >> I think when we went through this before (in NIR) we couldn't use {} >> because it needs to compile with MSVC, and MSVC doesn't allow that in >> C. >> > Precisely. The "most" portable solution that I know of is memset(). Except memset generates different code. That will clear the padding area between fields, and x = {} (and variations there of) do not. >>> Would be great to have the issue reported (and fixed) in Clang though. >>> AFAICT both {0} and {0,} are valid, if memory serves me right. >> >> I don't really understand the nuances of {0}, {0,}, {{0}}, etc, and I >> get the sense that that's the case for nearly everyone else as well. >> > AFAICT {0} and {0,} should just work everywhere, modulo bugs. And I believe most of these generate warnings if you enable other warning options (see also this patch series https://patchwork.freedesktop.org/patch/68141/). > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119 > [2] https://bugs.llvm.org/show_bug.cgi?id=21689 > >> The most prominent bug I see in all of this is that {} isn't part of >> standard C (it is part of standard C++!) > Agreed. Not sure how easy it will be to convince the committee. > > -Emil > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer
On 13 September 2017 at 23:31, Ian Romanick wrote: > On 08/29/2017 10:19 AM, Emil Velikov wrote: >> On 29 August 2017 at 18:10, Matt Turner wrote: >>> On Tue, Aug 29, 2017 at 3:35 AM, Emil Velikov >>> wrote: On 29 August 2017 at 11:11, Eric Engestrom wrote: > On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote: >> Avoids Clang's warning about the current code: >> >>warning: suggest braces around initialization of subobject > > I'm not sure about this patch [1], but everything else in this series is: IIRC "{}" is the least likely way to avoid the warnings, across GCC/Clang versions. It's not part of the C spec, but since it works - not sure how much to bother. >>> >>> I'm surprised to hear you say that. I've tested with gcc-4.9.4, 6.4.0, >>> 7.1.0; and clang-4.0, and none of them warn about {} >>> >> Eric corrected me on IRC, but I forgot to mention here - "s/least/most/" >> >>> I think when we went through this before (in NIR) we couldn't use {} >>> because it needs to compile with MSVC, and MSVC doesn't allow that in >>> C. >>> >> Precisely. The "most" portable solution that I know of is memset(). > > Except memset generates different code. That will clear the padding > area between fields, and x = {} (and variations there of) do not. > Would be great to have the issue reported (and fixed) in Clang though. AFAICT both {0} and {0,} are valid, if memory serves me right. >>> >>> I don't really understand the nuances of {0}, {0,}, {{0}}, etc, and I >>> get the sense that that's the case for nearly everyone else as well. >>> >> AFAICT {0} and {0,} should just work everywhere, modulo bugs. > > And I believe most of these generate warnings if you enable other > warning options (see also this patch series > https://patchwork.freedesktop.org/patch/68141/). > Even with maximum amount of warning flags, what gets flagged depends on compiler + version. Aka the "module bugs" mentioned above - feel free to skim through the bug reports linked. That said - yes, handling these is rather annoying :-\ >> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119 >> [2] https://bugs.llvm.org/show_bug.cgi?id=21689 >> -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer
On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote: > Avoids Clang's warning about the current code: > >warning: suggest braces around initialization of subobject I'm not sure about this patch [1], but everything else in this series is: Reviewed-by: Eric Engestrom The "mark `UNUSED` because they are unused in some generations" patches might be better to mark as `MAYBE_UNUSED`; they both resolve to the same thing for compilers, but the latter is gives more information to humans (ie. "this isn't always unused"). [1] Can you test this with GCC 4.x and 7.x? If memory serves, this init format caused warnings on either old GCC or new GCC. I think `= {{0}};` was the only format that both compilers were happy with. CC'ing Emil who might remember more/better than me. > --- > src/intel/vulkan/anv_formats.c | 2 +- > src/intel/vulkan/anv_pipeline.c | 20 ++-- > 2 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/src/intel/vulkan/anv_formats.c b/src/intel/vulkan/anv_formats.c > index 9808508523..6ce609ae8d 100644 > --- a/src/intel/vulkan/anv_formats.c > +++ b/src/intel/vulkan/anv_formats.c > @@ -739,7 +739,7 @@ VkResult anv_GetPhysicalDeviceImageFormatProperties2KHR( > *the implementation for use in vkCreateImage, then all members of > *imageFormatProperties will be filled with zero. > */ > - base_props->imageFormatProperties = (VkImageFormatProperties) {0}; > + base_props->imageFormatProperties = (VkImageFormatProperties) {}; > } > > return result; > diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c > index 279d76561a..8306cd366a 100644 > --- a/src/intel/vulkan/anv_pipeline.c > +++ b/src/intel/vulkan/anv_pipeline.c > @@ -510,7 +510,7 @@ anv_pipeline_compile_vs(struct anv_pipeline *pipeline, > } > > if (bin == NULL) { > - struct brw_vs_prog_data prog_data = { 0, }; > + struct brw_vs_prog_data prog_data = {}; >struct anv_pipeline_binding surface_to_descriptor[256]; >struct anv_pipeline_binding sampler_to_descriptor[256]; > > @@ -617,8 +617,8 @@ anv_pipeline_compile_tcs_tes(struct anv_pipeline > *pipeline, >pipeline->device->instance->physicalDevice.compiler; > struct anv_pipeline_bind_map tcs_map; > struct anv_pipeline_bind_map tes_map; > - struct brw_tcs_prog_key tcs_key = { 0, }; > - struct brw_tes_prog_key tes_key = { 0, }; > + struct brw_tcs_prog_key tcs_key = {}; > + struct brw_tes_prog_key tes_key = {}; > struct anv_shader_bin *tcs_bin = NULL; > struct anv_shader_bin *tes_bin = NULL; > unsigned char tcs_sha1[40]; > @@ -642,8 +642,8 @@ anv_pipeline_compile_tcs_tes(struct anv_pipeline > *pipeline, > } > > if (tcs_bin == NULL || tes_bin == NULL) { > - struct brw_tcs_prog_data tcs_prog_data = { 0, }; > - struct brw_tes_prog_data tes_prog_data = { 0, }; > + struct brw_tcs_prog_data tcs_prog_data = {}; > + struct brw_tes_prog_data tes_prog_data = {}; >struct anv_pipeline_binding tcs_surface_to_descriptor[256]; >struct anv_pipeline_binding tcs_sampler_to_descriptor[256]; >struct anv_pipeline_binding tes_surface_to_descriptor[256]; > @@ -775,7 +775,7 @@ anv_pipeline_compile_gs(struct anv_pipeline *pipeline, > } > > if (bin == NULL) { > - struct brw_gs_prog_data prog_data = { 0, }; > + struct brw_gs_prog_data prog_data = {}; >struct anv_pipeline_binding surface_to_descriptor[256]; >struct anv_pipeline_binding sampler_to_descriptor[256]; > > @@ -853,7 +853,7 @@ anv_pipeline_compile_fs(struct anv_pipeline *pipeline, > } > > if (bin == NULL) { > - struct brw_wm_prog_data prog_data = { 0, }; > + struct brw_wm_prog_data prog_data = {}; >struct anv_pipeline_binding surface_to_descriptor[256]; >struct anv_pipeline_binding sampler_to_descriptor[256]; > > @@ -976,7 +976,7 @@ anv_pipeline_compile_cs(struct anv_pipeline *pipeline, > } > > if (bin == NULL) { > - struct brw_cs_prog_data prog_data = { 0, }; > + struct brw_cs_prog_data prog_data = {}; >struct anv_pipeline_binding surface_to_descriptor[256]; >struct anv_pipeline_binding sampler_to_descriptor[256]; > > @@ -1277,8 +1277,8 @@ anv_pipeline_init(struct anv_pipeline *pipeline, > > pipeline->active_stages = 0; > > - const VkPipelineShaderStageCreateInfo *pStages[MESA_SHADER_STAGES] = { 0, > }; > - struct anv_shader_module *modules[MESA_SHADER_STAGES] = { 0, }; > + const VkPipelineShaderStageCreateInfo *pStages[MESA_SHADER_STAGES] = {}; > + struct anv_shader_module *modules[MESA_SHADER_STAGES] = {}; > for (uint32_t i = 0; i < pCreateInfo->stageCount; i++) { >gl_shader_stage stage = ffs(pCreateInfo->pStages[i].stage) - 1; >pStages[stage] = &pCreateInfo->pStages[i]; > -- > 2.13.5 > ___ mesa-dev mailing list mesa-dev@lists.freedesk
Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer
On 29 August 2017 at 11:11, Eric Engestrom wrote: > On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote: >> Avoids Clang's warning about the current code: >> >>warning: suggest braces around initialization of subobject > > I'm not sure about this patch [1], but everything else in this series is: IIRC "{}" is the least likely way to avoid the warnings, across GCC/Clang versions. It's not part of the C spec, but since it works - not sure how much to bother. Would be great to have the issue reported (and fixed) in Clang though. AFAICT both {0} and {0,} are valid, if memory serves me right. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer
On Tue, Aug 29, 2017 at 3:35 AM, Emil Velikov wrote: > On 29 August 2017 at 11:11, Eric Engestrom wrote: >> On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote: >>> Avoids Clang's warning about the current code: >>> >>>warning: suggest braces around initialization of subobject >> >> I'm not sure about this patch [1], but everything else in this series is: > IIRC "{}" is the least likely way to avoid the warnings, across > GCC/Clang versions. > It's not part of the C spec, but since it works - not sure how much to bother. I'm surprised to hear you say that. I've tested with gcc-4.9.4, 6.4.0, 7.1.0; and clang-4.0, and none of them warn about {} I think when we went through this before (in NIR) we couldn't use {} because it needs to compile with MSVC, and MSVC doesn't allow that in C. > Would be great to have the issue reported (and fixed) in Clang though. > AFAICT both {0} and {0,} are valid, if memory serves me right. I don't really understand the nuances of {0}, {0,}, {{0}}, etc, and I get the sense that that's the case for nearly everyone else as well. The most prominent bug I see in all of this is that {} isn't part of standard C (it is part of standard C++!) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer
On Tue, Aug 29, 2017 at 3:11 AM, Eric Engestrom wrote: > On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote: >> Avoids Clang's warning about the current code: >> >>warning: suggest braces around initialization of subobject > > I'm not sure about this patch [1], but everything else in this series is: > Reviewed-by: Eric Engestrom > > The "mark `UNUSED` because they are unused in some generations" patches > might be better to mark as `MAYBE_UNUSED`; they both resolve to the same > thing for compilers, but the latter is gives more information to humans > (ie. "this isn't always unused"). > > [1] > Can you test this with GCC 4.x and 7.x? If memory serves, this init format > caused warnings on either old GCC or new GCC. > I think `= {{0}};` was the only format that both compilers were happy with. Tested 4.9.4 and 7.1.0. No warnings from either of them about these initializers :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer
On 29 August 2017 at 18:10, Matt Turner wrote: > On Tue, Aug 29, 2017 at 3:35 AM, Emil Velikov > wrote: >> On 29 August 2017 at 11:11, Eric Engestrom wrote: >>> On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote: Avoids Clang's warning about the current code: warning: suggest braces around initialization of subobject >>> >>> I'm not sure about this patch [1], but everything else in this series is: >> IIRC "{}" is the least likely way to avoid the warnings, across >> GCC/Clang versions. >> It's not part of the C spec, but since it works - not sure how much to >> bother. > > I'm surprised to hear you say that. I've tested with gcc-4.9.4, 6.4.0, > 7.1.0; and clang-4.0, and none of them warn about {} > Eric corrected me on IRC, but I forgot to mention here - "s/least/most/" > I think when we went through this before (in NIR) we couldn't use {} > because it needs to compile with MSVC, and MSVC doesn't allow that in > C. > Precisely. The "most" portable solution that I know of is memset(). >> Would be great to have the issue reported (and fixed) in Clang though. >> AFAICT both {0} and {0,} are valid, if memory serves me right. > > I don't really understand the nuances of {0}, {0,}, {{0}}, etc, and I > get the sense that that's the case for nearly everyone else as well. > AFAICT {0} and {0,} should just work everywhere, modulo bugs. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119 [2] https://bugs.llvm.org/show_bug.cgi?id=21689 > The most prominent bug I see in all of this is that {} isn't part of > standard C (it is part of standard C++!) Agreed. Not sure how easy it will be to convince the committee. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer
On 29 August 2017 18:11:45 BST, Matt Turner wrote: > On Tue, Aug 29, 2017 at 3:11 AM, Eric Engestrom > wrote: > > On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote: > >> Avoids Clang's warning about the current code: > >> > >>warning: suggest braces around initialization of subobject > > > > I'm not sure about this patch [1], but everything else in this > series is: > > Reviewed-by: Eric Engestrom > > > > The "mark `UNUSED` because they are unused in some generations" > patches > > might be better to mark as `MAYBE_UNUSED`; they both resolve to the > same > > thing for compilers, but the latter is gives more information to > humans > > (ie. "this isn't always unused"). > > > > [1] > > Can you test this with GCC 4.x and 7.x? If memory serves, this init > format > > caused warnings on either old GCC or new GCC. > > I think `= {{0}};` was the only format that both compilers were > happy with. > > Tested 4.9.4 and 7.1.0. No warnings from either of them about these > initializers :) Given this and Emil's comment, this patch is r-b me with the rest of the series :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev