Skip to content

Conversation

vporpo
Copy link

@vporpo vporpo commented Jul 16, 2024

This helps avoid circular dependencies in a follow-up patch.

@vporpo vporpo requested review from slackito, tschuett and aeubanks July 16, 2024 18:29
#include "llvm/Support/raw_ostream.h"

namespace llvm {
namespace sandboxir {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace llvm::sandboxir {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not. llvm::Use could be forward declared. IIRC raw_ostream can also be forward declared.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we forward declare llvm::Use?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get rid of the include.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including llvm IR header files is not going to cause any dependency issues, so I think it should be fine to keep it this way.

void dump(raw_ostream &OS) const;
void dump() const;
#endif // NDEBUG
};
Copy link

@tschuett tschuett Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new line would be nice, but ...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I will add one.

@@ -0,0 +1,51 @@
#ifndef LLVM_TRANSFORMS_SANDBOXIR_USE_H
#define LLVM_TRANSFORMS_SANDBOXIR_USE_H

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not in the transforms directory. Maybe there was a version in transforms. The include guard looks odd.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops good catch!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also missing the LLVM header!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TRANSFORMS was copy-pasted from SandboxIR.h.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, let me fix that too.

This helps avoid circular dependencies in a follow-up patch.
@vporpo vporpo merged commit d01d9ab into llvm:main Jul 16, 2024
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary: This helps avoid circular dependencies in a follow-up patch.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251433
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants