FW: [PATCH] Some code improvements (no functional change)

[Artyom Skrobov]

Pinging this again, seems to have run cold over a month ago...


-----Original Message-----
From: Artyom Skrobov [mailto:Artyom.Skrobov at arm.com] 
Sent: 29 May 2014 11:42
To: 'llvm-commits'; 'Duncan P. N. Exon Smith'
Subject: FW: [PATCH] Some code improvements (no functional change)

Hello,

This is a weekly ping regarding the two suggested code changes as outlined
below (include guards in private headers, and renaming proposition for
SwapValue and SwapByteOrder)


-----Original Message-----
From: Artyom Skrobov [mailto:Artyom.Skrobov at arm.com] 
Sent: 22 May 2014 17:07
To: 'Duncan P. N. Exon Smith'
Cc: 'llvm-commits'
Subject: RE: [PATCH] Some code improvements (no functional change)

Hello Duncan,

Besides pinging on the discussion on include guards in private headers,
quoted below, I'd like to ask your opinion on naming the byte-swapping
functions swapByteOrder for the in-place variants, and getBytesSwapped for
the by-value/return variants; what do you think?
In addition to adhering to the lowercase-initial convention advised in the
Coding Standards, I believe this would also reflect more clearly what the
functions are doing, than their current names of SwapValue and
SwapByteOrder.


-----Original Message-----
From: Artyom Skrobov [mailto:Artyom.Skrobov at arm.com] 
Sent: 13 May 2014 18:12
To: 'Duncan P. N. Exon Smith'
Cc: llvm-commits
Subject: RE: [PATCH] Some code improvements (no functional change)

> It seems wrong to me that you're including regex2.h, as it looks like
private
> implementation details for the BSD regex code.  Adding include guards
makes
> the headers look less like shared (but private) implementation details and
> more like public interfaces -- that's misleading to anyone reading the
code.
> I still think these should be left as is, as the current setup discourages
> anyone from including them.

Curiously enough, lib/Support/regex_impl.h is (and has always been) guarded
by #ifndef _REGEX_H_ -- that's although the header's own name indicates it's
a part of the implementation, and not an interface (public nor private).
I've tried, and removing that guard doesn't have any effect on LLVM
compilation; I can conclude that the historic (non-)usage of include guards
in the regex headers isn't consistent enough to be taken as a guidance. 

Out of the 395 headers in lib/, supposedly none of them belonging to LLVM
public interface, 384 do have re-include guards, and only 11 don't (that's
lib/Support/Windows/WindowsSupport.h lib/Target/ARM/ARMPerfectShuffle.h
lib/Target/ARM64/ARM64PerfectShuffle.h
lib/Target/Hexagon/HexagonVarargsCallingConvention.h
lib/Target/NVPTX/NVPTXISelDAGToDAG.h
lib/Target/NVPTX/NVPTXMachineFunctionInfo.h
lib/Target/PowerPC/PPCPerfectShuffle.h and the 4 regex headers in question)
-- I think this shows a prominent tendency to have such guards in private
headers.

> However, I'm kind of new around here (maybe it's *my* ideas that are
wrong),
> and it's not like you're proposing something outrageous.
>
> Anyone else care to chime in?