I don't think it should be possible to attach metadata to constants owned by the LLVMContext (such as i32 0) since that seems like a bug, but IIUC this patch is enabling that. Is that intentional? If so, why? If not, can you find a way to avoid making that possible?

In D67568#1670131, @steven_wu wrote:

In D67568#1670049, @kledzik wrote:

How many symbols are there today? Do we want this overhead of a function call for each? Could you instead return a count and array pointer (like argc and argv) or have a "foreach" function that takes a block and calls it back for each symbol?

I am just following the old convention. The number of symbols are around 500. foreach is not that cheaper because it is one function call per iteration. I am fine with either solution.

LGTM. I have one idea for you to consider inline.

I don't think you need prior frontend expertise, just some time and patience since the modules code has some technical debt. If you are still willing to look at it, I agree with Richard's suggestion that we could merge this with the map in the Module Manager. One approach would be to start caching (and invalidating?) module load failures in the ModuleManager somehow, redirect APIs using KnownModules to point there, and then delete this cache entirely. Another approach would be to re-evaluate if we need to cache module load failures; hypothetically, it's possible we don't need that feature (anymore).

I went back to read notes from when we deployed -Wstrict-prototypes (which we have had on-by-default for our users for a couple of years, since it catches lots of -fblocks-related bugs). I was mainly objecting because I had (mis)remembered trying and backing out the specific behaviour you're adding. On the contrary, our notes say that we might want to strengthen the diagnostic as you're doing.

A few minor comments. Assuming the FIXME I point out was intentionally left for later, this LGTM.

r370546.

In D66919#1650174, @dexonsmith wrote:

We could carve out a -W flag (if it doesn't already exist) that warns if you incorrectly pass parameters to a function whose definition has no prototype

The warning exists, but there is no flag apparently.

void f() {}
void g() {
    f(0);
}

spits out

test.c:3:8: warning: too many arguments in call to 'f'
    f(0);
    ~  ^

r370488

LGTM.

In D66919#1650775, @aaron.ballman wrote:

In D66919#1650174, @dexonsmith wrote:

This could cause a lot of churn in existing projects (especially with static void foo()), without giving Clang any new information. I'm wary of this.

Those projects likely aren't aware they're using prototypeless functions, which are trivial to call incorrectly. I suspect this diagnostic will find real bugs in code.

This could cause a lot of churn in existing projects (especially with static void foo()), without giving Clang any new information. I'm wary of this.

Added a FileManagerTest and changed FileManager::getBypassFile to use FileEntryRef.

This LGTM.

LGTM, thanks!

In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

Is this still a problem?

In D66556#1647762, @dexonsmith wrote:

In D66556#1646229, @aganea wrote:

This failed the build - I've changed unix2dos to svn:eol-style CRLF instead.

Can we count on tr when we have shell? If so, I suggest this instead:

// REQUIRES: shell
// RUN: tr '\n' '\r\n' <%s | %clang_cc1 -DOTHER -print-dependency-directives-minimized-source 2>&1 | FileCheck %s

In D66556#1646229, @aganea wrote:

This failed the build - I've changed unix2dos to svn:eol-style CRLF instead.

In D66443#1643395, @mgehre wrote:

Generally, ArrayRef is also a candidate. But there we have the complication that OwningArrayRef indirectly inherits from ArrayRef. One is a gsl::Owner, the other a gsl::Pointer, and we
need to make sure that we handle it correctly when OwningArrayRef is accessed through its base class which is then seen as a Pointer.
I would thus defer this part to a later PR.

Committed in r369958.

New diff with full context.

r369943.

[Marking as "Request Changes" to get this out of my queue.]

Abandoning this in light of the recent discussions.

(Updating the diff with full context.)

Fix the change to CompilerInstance.cpp.

Also update FileEntryRef to be copy- and move-assignable so that Optional<FileEntryRef> can be assigned to.

Updated patch after running check-clang and learning more about Expected. I've added a parallel API using Optional<FileEntryRef> for clients that don't want to do anything with the error.

@arphaman, is there a reason you think ErrorOr is more appropriate long-term here?

LGTM, with a minor suggestion inline.

In D66195#1629514, @hans wrote:

In D66195#1629329, @jfb wrote:

In D66195#1629120, @hans wrote:

Perhaps add a note in docs/ReleaseNotes.rst as well?

I can, but that seems odd: aren’t release notes for users of LLVM, not developers? In other words, what would someone reading this note take away from it, and do based on reading it?

I think the releases are also used by folks doing various kinds of out-of-tree LLVM stuff, and this change might let them use C++14 in their code by default.

LGTM.

In D65906#1621542, @aganea wrote:

In D65906#1620034, @arphaman wrote:

Regarding the invisible characters before #ifdef, are you sure that's the right behavior?

Should we then copy these invisible characters to the minimized output? Believe it or not, these characters are there in our codebase since 2013, and never clang has complained about it :)

LGTM after you add the testcase I mention.

LGTM, but I have a suggestion inline for another approach.

Is warning output suppressed? If so, should we just/also disable all warnings? (IIRC, the flag is -w.)

In D63048#1558806, @sylvestre.ledru wrote:

@dexonsmith ping?

In rL336869#663240, @twoh wrote:

Hello, can you please explain a bit more about this change? What kind of redundant outputs avoided by this? Thanks!

In D62742#1540426, @dblaikie wrote:

I don't fully have the direction you've in mind/that this is a part of in my head - but roughly enough to give a thumbs-up here :)

In D62428#1525189, @mclow.lists wrote:

In D62428#1519664, @jfb wrote:

In D62428#1518488, @mclow.lists wrote:

The deletions are fine, the doc updates are good, but I think deprecation warnings are not.
Things that are going to happen in the future (a year from now, say) or never (if people don't update their tools) don't belong in the warning spew for every single build

In D60713#1523315, @aprantl wrote:

Prior to this patch Function declarations may not have a !dbg attachment, because there was no use-case for it. With call site debug info, I think it would make sense to revisit this rule. When I made my previous comment, I thought that the updated rule could be:

(1) function definitions may have a distinct DISubprogram attached (as before)
(2) function declarations may have a unique DISubprogram attached (new)

If I read the code in Verifier.cpp correctly, then it is legal for a unique DISubprogram to not have a unit, which would cause the declaration-DISubprograms to be uniqued during LTO.

so the (2) should probably be:

(2) function declarations may have a unique DISubprogram attached and it may not have a unit: field

I've CC'ed Duncan in case he can catch a logic error with this reasoning.

In D62618#1521964, @EricWF wrote:

Changing the triviality of a type can be ABI breaking because it will change the calling conventions the compiler uses.
We have hacks in pair to keep it non-trivial on some platforms for this reason.

This doesn’t look okay to me, because this would prevent building for Darwin when running on Windows. I added a couple of reviewers that have Windows experience and might have ideas for how to fix the tests without breaking the portability of the driver.