Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)

From: Julien Grall <julien@xen.org>
To: Elliott Mitchell <ehem+xen@m5p.com>, Ian Jackson <iwj@xenproject.org>
Cc: xen-devel@lists.xenproject.org, Julien Grall <jgrall@amazon.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Jan Beulich <jbeulich@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Dario Faggioli <dfaggioli@suse.com>, Tim Deegan <tim@xen.org>,
	Roger Pau Monn?? <roger.pau@citrix.com>,
	Anthony PERARD <anthony.perard@citrix.com>
Subject: Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)
Date: Tue, 6 Apr 2021 18:55:12 +0100	[thread overview]
Message-ID: <2e51a7e5-404f-fd2e-90cb-c4d94c4d807b@xen.org> (raw)
In-Reply-To: <YGtCTfw7WmOJPaa/@mattapan.m5p.com>

Hi Elliott,

Thanks for the input!

On 05/04/2021 18:01, Elliott Mitchell wrote:
> On Mon, Apr 05, 2021 at 04:56:59PM +0100, Julien Grall wrote:
>> I am not aware of code trying to modify literal strings in Xen.
>> However, there is a frequent use of non-const char * to point to
>> literal strings. Given the size of the codebase, there is a risk
>> to involuntarily introduce code that will modify literal strings.
>>
>> Therefore it would be better to enforce using const when pointing
>> to such strings. Both GCC and Clang provide an option to warn
>> for such case (see -Wwrite-strings) and therefore could be used
>> by Xen.
>>
>> This series doesn't yet make use of -Wwrite-strings because
>> the tree is not fully converted. Instead, it contains some easy
>> and likely non-controversial use const in the code.
>>
>> The major blockers to enable -Wwrite-strings are the following:
>>      - xen/common/efi: union string is used in both const and
>>      non-const situation. It doesn't feel right to specific one member
>>      const and the other non-const.
>>      - libxl: the major block is the flexarray framework as we would use
>>      it with string (now const char*). I thought it would be possible to
>>      make the interface const, but it looks like there are a couple of
>>      places where we need to modify the content (such as in
>>      libxl_json.c).
>>
>> Ideally, I would like to have -Wwrite-strings unconditionally used
>> tree-wide. But, some of the area may required some heavy refactoring.
>>
>> One solution would be to enable it tree-wide but turned it off at a
>> directroy/file level.
>>
>> Any opinions?
> 
> I think doing such is a Good Idea(tm).  Adding consts adds opportunities
> for greater optimization.  Both by compilers which can avoid extra
> copies, and developers who then know they don't need to generate extra
> copies to sacrific them to an API.  In particular the consts also
> function as documentation.
> 
> So you're certainly not the only person who thinks additional consts
> would be a good thing:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2021-01/msg00132.html
> 
> 
> Alas merely getting the two const patches into the latest release
> apparently wasn't even worthy of response:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2021-02/msg01040.html

Sory to hear you had no response. Would you be able to give a nudge to 
the maintainers?

> 
> 
> I agree this should be done, though I'm aware many developers hate
> bothering with constants.
> 
> 

Cheers,

-- 
Julien Grall