[PATCH] Some code improvements (no functional change)

Artyom Skrobov Artyom.Skrobov at arm.com
Tue May 13 10:11:35 PDT 2014


Thanks Duncan

>>> Yes, this looks better.  Send the full patch and I'll have another look.
>> 
>> Thank you! Attaching the updated patch.
>
> LGTM.

Committed as r208680 and r208684

> Note that the change to include/llvm/Support/Unicode.h still LGTM.  Commit
> that any time.

Committed as r208681

> 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?








More information about the llvm-commits mailing list