From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Deucher Subject: Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c Date: Thu, 21 Mar 2019 00:44:08 -0400 Message-ID: References: <20190320005406.GA16412@archlinux-ryzen> <20190320043440.GA23335@archlinux-ryzen> <63518f1f-b808-77b0-aac6-ee1ece669c4b@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Nick Desaulniers Cc: "Koenig, Christian" , "Zhou, David(ChunMing)" , "Pan, Xinhui" , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "clang-built-linux@googlegroups.com" , "amd-gfx@lists.freedesktop.org" , "Deucher, Alexander" , Nathan Chancellor List-Id: dri-devel@lists.freedesktop.org On Wed, Mar 20, 2019 at 4:49 PM Nick Desaulniers wrote: > > On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian > wrote: > > > > Am 20.03.19 um 05:34 schrieb Nathan Chancellor: > > > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote: > > >> these two enumerated types are same for now. both of them might change in the future. > > Please consider if the YAGNI principle applies here. > https://martinfowler.com/bliki/Yagni.html > > > >> > > >> I have not used clang, but would .block_id = (int)head->block fix your warning? If such change is acceptable, I can make one then. > > My preference on solutions, in order: > 1. One enum (this is the simplest most type safe solution). Add > another enum when you actually need it. Otherwise, YAGNI. It make sense to have two enums. One is a firmware interface that is only used by some asics and the other is for the general driver interface (non-asic specific for the ras features. > 2. Safe casting function (like the one Nathan supplied, maybe with > WARN_ONCE rather than WARN). This ensures that at least if the types > diverge you get a runtime warning. A compile-time warning would be > preferred, but I haven't taken the time to think through how that > might be implemented. I'd prefer this one. Alex > 3. Cast to int (this has been used in other places throughout the > kernel, but provides the weakest type safety and doesn't catch future > divergence). > 4. Disabling the warning. (I almost never advocate for this). > > > Another question is if it is such a good idea to just silence the warning? > > For the kernel, it seems that each maintainer can choose what to apply > to their subsystem. I would recommend against disabling additional > warnings that aren't disable kernel-wide for most cases. > -Wenum-conversion has spotted many bugs. While the enums in question > today are not different, they MIGHT eventually diverge and lead to > bugs, like the others we've found and fixed throughout the kernel. So > I would recommend fixing now, and be insulated in the future. > > -- > Thanks, > ~Nick Desaulniers > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx