[PATCH] D18469: [LLVM] Fix Clang-tidy modernize-deprecated-headers warnings in some files; other minor fixes

Mehdi Amini via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 25 11:36:19 PDT 2016


> On Mar 25, 2016, at 11:28 AM, Eugene Zelenko <eugene.zelenko at gmail.com> wrote:
> 
> Eugene.Zelenko added inline comments.
> 
> ================
> Comment at: include/llvm/DebugInfo/PDB/PDBTypes.h:18
> @@ -18,1 +17,3 @@
> +#include <cstdint>
> +#include <cstring>
> 
> ----------------
> joker.eph wrote:
>> Eugene.Zelenko wrote:
>>> joker.eph wrote:
>>>> Eugene.Zelenko wrote:
>>>>> joker.eph wrote:
>>>>>> why do we need cstring here? (You uploaded the patch without context, which makes it hard to figure here)
>>>>> Because of strcpy().
>>>> Note sure if it is legit to have this in a header. This header is included in many many places!
>>> But strcpy() is used in this header.
>>> 
>>> This is whole idea of Include What You Use: to make files dependencies self-containing, so there is no need to rely on other headers.
>> I'm not sure you got my point: "having the function that is using strcpy() in this header is not a good idea". Small header is better for compile time, and complicated methods can be defined in implementation file (when not templated).
> Sorry for misunderstanding.
> 
> Will be good idea to address this question to original authors.

Sure, I know you didn't introduce this code, you just made the dependency on the cstring header explicit.

> Probably the wanted to use implicit inlining (depended on optimization level).

Performance is rarely a good motivation: LTO build is there for that.

(You noticed I approved your patch yesterday right?)


-- 
Mehdi



More information about the llvm-commits mailing list